hidden field ignore rule for builder setter methods

[Roman Ivanov]

wigbam commented on Aug 24

Currently HiddenField module has an option to ignore only simple setter methods returning void. Assume the checkstyle.cfg has the following:

...
<module name="HiddenField">
    <property name="ignoreSetter" value="true"/>
</module>
...

Then this source file would be flagged:

class MyBuilder {
    String prop;
    public void setProp(String prop) { // this gets correctly ignored due to 'ignoreSetter'
        this.prop = prop;
    }

    public MyBuilder withProp(String prop) { // this gets flagged
        this.prop = prop;
        return this;
    }
}

The proposed improvement is to add another boolean property 'ignoreBuilderSetter' to HiddenField module which would for all types named as .*Builder allow to ignore setter methods named either withX or setX and returning the defining type.

See committed xdoc for details.

==============================

romani commented on Aug 24

1) please squash all commits in one.
2) I do not like that Builder definition is hard-coded, it should be setup by user.
3) why mIgnoreSetter does not work for you ? (please provide detailed description of code that you have, config that you use and what you would like to have)

===========================

romani commented on Aug 25

That for description ,now It see clearly what is your intend.

2) I do not like that Builder definition is hard-coded, it should be setup by user.

you missed that point to fix.
Consider builder example: https://docs.jboss.org/hibernate/orm/3.3/reference/en-US/html/querycriteria.html
if you rely on name to distinguish "Builder" do not assume that will always be named like this. User have to define it, but you can give good default values.

(mIgnoreBuilderSetter ? "(set|with)" : "set")

Why you think that with set and with are the only names that could be used in Builder, see link above?

===================================

wigbam commented on Aug 26

Ok, to have more flexibility over the configuration only makes sense. But a bit more work will be required then.
Before I move further and outline the proposed changes and in order to avoid possible confusion, I would like to clarify that in the mentioned Hibernate's Criteria class, method add(Restriction) is not a valid setter method, while setMaxResults(int) is.

Now, in order to accommodate the required features, instead of a newignoreBuilderSetter property several other properties could be added:

Property Name	Description	Default Value	Example Setting
setterPrefixRegex	The regular expression defining acceptable prefixes for setter methods. The method will considered to be a valid setter method if its name matches the regular expression ^<setterPrefixRegex><cFieldName>$, where cFieldName is the capitalized name of the variable being set	set	(set|with)
setterCanReturnItsTypeIn (or suggest an alternative name)	The regular expression defining what type names are allowed to have setter methods return their defining type.	^$	.*Builder

Let me know what you think.

=====================================

romani commented 29 days ago

@wigbam , looks closer to true but .....

Lets come back to original problem. You have problem with setter recognition, so you need to have option that allow you to specify what prefix should mark method as "setter".
In you approach we try to specify set of classes(regexp) where that setter recognition is custom.
But what if have few types (.*Builder with "with.*" and .*Creator with "add.*") in in each of them its own custom prefix that mean setter.
So as we would have one option for Prefix and ClassName we need to combine regexp to cover all, so we might have false-positives between Builder and Creator groups (in Creator with is not a setter).
So we come back to original idea that setter recognition should be general for all classes. Update is simple - just implement setterPrefixRegex. But this is also give your missed cases when method "withItem" appear in non Builder class.

term "setter" is highly debatable in Java - it is up to a user , what is setter and what is not. So I think that just implementing setterPrefixRegex will be enough.

do you follow me ?

====================================

wigbam commented 24 days ago

@romani, not really, the original problem was to do with Checkstyle's expectation for setters to return void. Therefore, simply implementing setterPrefixRegex would not be enough for builders where, typically, setter methods return themselves.
The existing logic could be amended to ignore return types of setters, but that would be a breaking change, which is bad for obvious reasons. The proposal I made earlier would allow us to keep current functionality, while adding the new features on top.

======================================

romani commented 17 days ago

@wigbam, one more idea:
It look to me that we need only one option that detect Builders classes. And we will ignore by that type name all inner methods that return that Builder type - as in builder all methods that return himself is kind of Setter (no extra options for detecting "with" prefix is required).

So option could be :
ignoreSetterInBuilderTypes="*Builder$"
in description (javadoc) of that option we need describe in detail how it works

Does it make sense ?

============================================

priimak commented 8 hours ago

Hi All.

But isn't it is common to have setters in the form

Foo setX(int x) {
    this.x = x;
    return this;
}

in classes that do not nessesaerly named ".*Builder$"?

I would say that if method satisfy following rules:

It is named (using perl regex) /set(.*)\([^\S,]\s+\1\)/ and it returns either void 
or class is in which it is defined or its parent class ( assuming that @override is set )

Then such method should be considered a setter. However in order to maintain backward
compatibily we need to keep meaning of ignoreSetter property intact. Therefore, I would propose
that we call such setters "Exetended Setter" and add following property

<property name="ignoreExtectedSetter" value="true"/> 

to the HiddenField module.

================================================

[Roman Ivanov]

Hi priimak,

> in classes that do not nessesaerly named ".*Builder$"?

user will have full control over that property - you need, to update it to be ".*(Builder|Foo)$" in your case. 

> or class is in which it is defined or its parent class

Checkstyle for now have no ability to look out of analyzed file for "parent class", so it is not reliable approach.

> /set(.*)\([^\S,]\s+\1\)

relying only on name for setting calculation is not reliable approach, too much false positives will be with names. We experienced that problem during testing of CustomDeclarationOrderCheck .

thanks,

Roman Ivanov

[Dmitri Priimak]

I see, but have one concern here. I am under impression that HiddenField module is agnostic
to the class name where its rules are applied. Setting property ignoreSetter to true makes it
applicable to any class where setters are declared. Relying on the property matching class names
would be very different from the current approach and it would mean mixing checkstyle rules with
specific code. Shouldn't there be a way to add property to the HiddenField module, like what I
proposed, i.e. ignoreExtendedSetter that would accomplish what we want. How it could
accomplish it I cannot really say right now, a regex match (but you say it is no good) or something
else. But I always felt uneasy about my own java code and checkstyle rules.

--
Dmitri Priimak

[Dmitri Priimak]

Or even better. Add new property that controls what is the setter. Lets call it 'setterType' with
possible values 'classic' ( default value corresponding to setter returning void ) or 'extended' were
setter can return either void or the class in which it is declared. This would look like so

        <module name="HiddenField">
            <property name="ignoreConstructorParameter" value="true"/>

            <property name="ignoreSetter" value="true"/>

            <property name="setterType" value="extended"/>
        </module>

What do you think?

--
Dmitri Priimak

[Roman Ivanov]

Hi Dmitri,

I will reply you in as soon as I finish Chekstyle 5.8 release. Sorry, I can not postpone that any more.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Dmitri,

I got your idea of option. Yes, you are right, your idea is better than all above.

But we will go with simple boolean option
ignoreBuilderSetter.

I do not like enums, as they cost more in further support, and name "extended" is not descriptive:
http://checkstyle.sourceforge.net/property_types.html#parenPad

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/PadOption.java

Thanks,
Roman Ivanov

[Dmitri Priimak]

Hi Roman.

Actually, I am just finishing implementing prototype of what  I proposed.
Would you guys be willing to take a look at it?

As far as name "extended", that can be changed to be mode descriptive.

Perhaps instead of 'setterType' the parameters could be 'setterReturning'
with default value 'void' or 'self'. Where 'self' would indicate that it could
return class where the setter is declared. Although I think that
setterType=extended is fine too if it is properly documented.

--

Dmitri Priimak

[Roman Ivanov]

Put a link at mail list for review, option should be boolean.

[Dmitri Priimak]

Hi All.

You can find my forked checkstyle with the changes that implement my
proposed mechanism of identifying setters that return _this_ here

    https://github.com/priimak/checkstyle

The only modified file is this one

    https://github.com/priimak/checkstyle/blob/71552917baaf7a54e8965b4531cbf92d8e33c6c8/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

The diff is here

    https://github.com/priimak/checkstyle/commit/71552917baaf7a54e8965b4531cbf92d8e33c6c8?diff=split

To try it out you must add property setterCanReturnThisClass to the HiddenField module
config in the checkstyle.xml like so

        <module name="HiddenField">

            <property name="ignoreSetter" value="true"/>

            <property name="setterCanReturnThisClass" value="true"/>
        </module>

By default setterCanReturnThisClass is false, which means that HiddenField module
works exactly is it now, unless setterCanReturnThisClass is set to true.

Please let me know if this is of any interest to you.

--

Dmitri Priimak

[Dmitri Priimak]

Hi All.

So, what do you guys think?

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmitri,

sorry, but I postpone you till finish 5.8 release, I can not be distracted forever by PRs and other project questions.

I will review it for sure after I do 5.8 release.

thanks,

Roman Ivanov

[Roman Ivanov]

Hi Dmitri,

https://github.com/priimak/checkstyle/blob/71552917baaf7a54e8965b4531cbf92d8e33c6c8/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

As you plan to do contribution to main project please expect that I will demand maximal testing level.

We need UTs.

We will need to test your changes on Guava library , and Hibernate and Spring.

You will need to generate maven checkstyle html report for that to prove that no false positives appear.

1) 

* An example of how to configure the check so that it ignores the parameter of

* a setter method is:

We need to define to user what is Setter, and provide code example.

2)

* An example of how to configure the check so that it ignores the parameter of

* a chain-setter method. A chain-setter is a setter that returns instance of

* the class in which it itself is declared.

We need to provide example of code to be exact.

Show that new option depend on old option.

3) 

> setterCanReturnThisClass

need to be renamed to setterCanReturnItsClass

4) 

* @author Rick Giles

* @version 1.0

Add yourself , remove version tag

5)

     * name 'setXyz', one parameter named 'xyz', and return type void.

please fix javadoc 

6)

// Does it return void or class in which it is declared?

final DetailAST typeAST = methodAST.findFirstToken(TokenTypes.TYPE);

if (!typeAST.branchContains(TokenTypes.LITERAL_VOID)) {

// return type is not void.

if (!mSetterCanReturnThisClass) {

Need to be optimized.

7) 

for (;;) {

if (classAST == null

|| classAST.getType() == TokenTypes.CLASS_DEF)

{

break;

}

classAST = classAST.getParent();

}

please use clear form of LOOP.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Dmitriy, 

please do reply to mail-list, as I am not alone at maintaining Chckstyle, other admins could have their thoughts too.

thanks,

Roman Ivanov

On 29 October 2014 20:49, Roman Ivanov <romani...@gmail.com> wrote:

Hi Dmitri,

https://github.com/priimak/checkstyle

1) please do squash for all your changes, it will ease code review.

https://github.com/priimak/checkstyle/blob/67f2cb77a5cade2ab8399a4efcd3eeb3fabc3cd0/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

1) please pull and rebase on latest master , I recommend to keep your changes in topic branch and do PR from that branch.

I see "</p>" in javadoc , but in java8 that tags cause compilation problems

2) 

> A method is recognized as a setter if it is in the following form

please describe first of what is general setter and then your perfect description of  builder. I like it, but it bit confusing with "void".

Please do not update xdoc till we come to agreement on javadoc content (that will economize you time)

3) 

 // go up until we encounter class definition

DetailAST classAST = methodAST.getParent();

I do not like that, please update FieldFrame or create wrapped Class to keep reference to class that is currently under investigation

4) 

 // we went all the way up but did not find class definition and

// therefore unable to verify if this return type is allowed in a

// setter or not. HiddenField check will not be ignored.

please show me code that will cause us to fail to find top class

5) 

> private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

I do not like amount of "returns" in that method, what do you think to make it as one-return method.

6) we need to test algorithm on open source. Please take a look at http://maxvetrenko.github.io/, sources are at https://github.com/maxvetrenko/guava-libraries-17-0 and https://github.com/maxvetrenko/maxvetrenko.github.io ,  http://roman-ivanov.blogspot.com/2014/10/how-to-use-snapshot-checkstyle-version.html .

Please create a report and lets review results together, if you think that is reasonable to test on bigger project - you are welcome.

thanks,

Roman Ivanov

​

[Dmitri Priimak]

On Wed, Oct 29, 2014 at 8:49 PM, Roman Ivanov <romani...@gmail.com> wrote:

Hi Dmitri,

https://github.com/priimak/checkstyle

1) please do squash for all your changes, it will ease code review.

https://github.com/priimak/checkstyle/blob/67f2cb77a5cade2ab8399a4efcd3eeb3fabc3cd0/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

Done. See two branches where these changes are collapsed into one.  

 

1) please pull and rebase on latest master , I recommend to keep your changes in topic branch and do PR from that branch.

I see "</p>" in javadoc , but in java8 that tags cause compilation problems

"</p>" closing tags exists in the code that preceded my changes. Do you want me to remove them?
 

 

2) 

> A method is recognized as a setter if it is in the following form

please describe first of what is general setter and then your perfect description of  builder. I like it, but it bit confusing with "void".

Please do not update xdoc till we come to agreement on javadoc content (that will economize you time)

I have updated javadoc content, hopefully making it easier to understand. Please let me know if it is still unsatisfactory.

 

3) 

 // go up until we encounter class definition

DetailAST classAST = methodAST.getParent();

I do not like that, please update FieldFrame or create wrapped Class to keep reference to class that is currently under investigation

Done.
 

4) 

 // we went all the way up but did not find class definition and

// therefore unable to verify if this return type is allowed in a

// setter or not. HiddenField check will not be ignored.

please show me code that will cause us to fail to find top class

You are right. The potential condition I am describing is impossible and I am trying to be overly cautious. This code is now removed.
 

5) 

> private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

I do not like amount of "returns" in that method, what do you think to make it as one-return method.

In this particular case making it one return method makes for a harder to understand code.
Thus I prefer the code the way it is written right now. But I have made a version where number
of return statements is slightly reduced. See this
branch https://github.com/priimak/checkstyle/tree/SimplifiedSetterTest

 

6) we need to test algorithm on open source. Please take a look at http://maxvetrenko.github.io/, sources are at https://github.com/maxvetrenko/guava-libraries-17-0 and https://github.com/maxvetrenko/maxvetrenko.github.io ,  http://roman-ivanov.blogspot.com/2014/10/how-to-use-snapshot-checkstyle-version.html .

Please create a report and lets review results together, if you think that is reasonable to test on bigger project - you are welcome.

I had some trouble forcing maven to use my version of checkstyle, it seem to alway
download com/puppycrawl/tools/checkstyle/5.7/checkstyle-5.7.jar from maven central.

Any ideas what could be wrong?

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Dmitriy,

Thanks or your persistence and time . I will provide code review a bit later.

> Any ideas what could be wrong?

Did you read http://roman-ivanov.blogspot.com/2014/10/how-to-use-snapshot-checkstyle-version.html ?

I am attaching pom.xml file that is working, at least for me and for now :) . Base version is https://github.com/maxvetrenko/guava-libraries-17-0

thanks,

Roman Ivanov

​

[Dmitri Priimak]

On Mon, Nov 17, 2014 at 5:35 PM, Roman Ivanov <romani...@gmail.com> wrote:

Hi Dmitriy,

Thanks or your persistence and time . I will provide code review a bit later.

No, no. Thank you.
 

> Any ideas what could be wrong?

Did you read http://roman-ivanov.blogspot.com/2014/10/how-to-use-snapshot-checkstyle-version.html ?

I am attaching pom.xml file that is working, at least for me and for now :) . Base version is https://github.com/maxvetrenko/guava-libraries-17-0

I am still having trouble here. I forked checkstyle when in pom.xml it had version number 5.8-SNAPSHOT. Thus I took
your pom.xml and changed following from

          <dependency>
            <groupId>com.puppycrawl.tools</groupId>
            <artifactId>checkstyle</artifactId>
            <version>6.1</version>
         </dependency>

to

         <dependency>
            <groupId>com.puppycrawl.tools</groupId>
            <artifactId>checkstyle</artifactId>
            <version>5.8-SNAPSHOT</version>
         </dependency>

However, when I run 'mvn site' it still downloads checkstyle 5.7 not 5.8-SNAPSHOT, which is  installed locally in ~/.m2/

Is there something else I am missing?

--

Dmitri Priimak

[Dmitri Priimak]

Hi.

So, I solved this problem by recompiling maven-checkstyle-plugin with dependency pointing at my version of checkstyle
built based on this branch https://github.com/priimak/checkstyle/tree/SimplifiedSetterTest

I cloned https://github.com/maxvetrenko/guava-libraries-17-0 and made some changes to checkstyle_google_style.xml

Specifically I added this check

        <module name="HiddenField">
          <property name="ignoreSetter" value="true"/>

          <property name="setterCanReturnItsClass" value="true"/>
          <property name="ignoreConstructorParameter" value="true"/>
          <property name="ignoreAbstractMethods" value="true"/>
        </module>

This clone is available here https://github.com/priimak/guava-libraries-17-0

then I run 'mvn site' and committed generated reports as well, i.e. targets/site/*

The file you will be interested in is https://github.com/priimak/guava-libraries-17-0/blob/master/target/site/checkstyle-aggregate.html

you can view as html by following this link

    https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-aggregate.html

One more thing. I am not sure if checkstyle now fully supports java 8, but this check relies on the fact that methods can only
be embedded in normal java classes. However, in java 8 it is possible to have default methods on interface. In my opinion it will
be trivial to extend this check to to be used with the default methods. Should that be of importance at the moment?

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmiriy,

https://github.com/priimak/checkstyle/blob/546582d017c22d275a42765441ce1e8f52ec89af/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

Looks like with such update we end up in refactoring whole Check .... as even testing will be serious no matter what.

0) please move all your changes to topic branches, pull our master to you master to let be on 6.1, as there was a lot of changes, but not in your Check ( you should not have any conflictsproblem).

1)

 * </p>

please do me favor - remove that from javadoc, for some kind of understandable reason java8 started to put a violation on close P tag.

2) 

 * Any other return types are disallowed. 

Any other return types will not let method match a setter pattern.

3) 

 * a setter is expanded, so that setter return type can also be a class in

what a bout enums ? in interface there is no members , but enums are kind of the same class , most likely used for Singletons 

4) 

/** stack of sets of field names,

* one for each class of a set of nested classes.

*/

private FieldFrame mCurrentFrame; lets refactor that to stack and rename class to ClassFrame. 5)  private String mInClassName; as we refactor that to stack , that field will not be required.

6) 

public void visitToken(DetailAST aAST)

we need to refactor that method to have 

switch() {

VARIABLES:

    processVariable()

CLASS and ENUM:

    processType()

}

too much is put in that methods 

7) 

private void processVariable(DetailAST aAST)

please make a one return, that will simplify method.

8) 

private boolean isMethodASetter(DetailAST aMethodAST, String aName)

I like you changes in logic, but please keep one return in method, there is no reason to have two there

9)

private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

please move that method to me located above isMethodASetter, as it is callled first.

10)

private static String capitalize(final String aName)

private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

I understand that is not your code , but please do me favor - lets keep one return point from method. 

11)

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-aggregate.html

please do me a favor , please activate only your Check , all other should be removed from config, report should be regenerated.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Dmitrii,

please use attached file , I did them base form your repo https://github.com/priimak/guava-libraries-17-0.

Report regerate by "mvn checkstyle:checkstyle" (without CSS), but I recommend to generate report as "mvn site".

I updated: http://roman-ivanov.blogspot.com/2014/10/how-to-use-snapshot-checkstyle-version.html

Please generate report with new option as FALSE and TRUE, attention: you do not have to have two copies of web site, the only difference will be in checkstyle.html  file only .

So, generate report ones (setterCanReturnItsClass=false), copy it to git repo that is IO as checkstyle-setter-false.html . Generate  report one more time with setterCanReturnItsClass=true, copy it to git repo that is IO as checkstyle-setter-true.html .

All other files should be not changed, you can recheck that yourself.

Review created reports yourself and send me two links that html file for review.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Dmitriy,

during testing we should be aware of https://github.com/checkstyle/checkstyle/issues/382

thanks,

Roman Ivanov

[Dmitri Priimak]

Also recognition of setters happens in the JavadocMethod module, which is in this file
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocMethodCheck.java

Interestingly check that happens in the JavadocMethodCheck::isSetterMethod(...) also looks inside of the setter method.

Perhaps these two checks should be combined.

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmitri,

JavadocMethodCheck will be completely reimplemented to use Javadoc Grammar for parsing, current implementation (regexp) is very error prone.

Each check is independent entity, they could share only util methods.

thansk,

Roman Ivanov

[Roman Ivanov]

ping ?

[Dmitri Priimak]

Sorry, forgot to include cc to checksty...@googlegroups.com

---------- Forwarded message ----------
From: Dmitri Priimak <pri...@gmail.com>
Date: Mon, Jan 19, 2015 at 2:14 AM
Subject: Re: hidden field ignore rule for builder setter methods
To: Roman Ivanov <romani...@gmail.com>

Ok. So here it is. See my inlined comments.

On Tue, Nov 18, 2014 at 7:13 PM, Roman Ivanov <romani...@gmail.com> wrote:

Hi Dmiriy,

https://github.com/priimak/checkstyle/blob/546582d017c22d275a42765441ce1e8f52ec89af/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

Looks like with such update we end up in refactoring whole Check .... as even testing will be serious no matter what.

0) please move all your changes to topic branches, pull our master to you master to let be on 6.1, as there was a lot of changes, but not in your Check ( you should not have any conflictsproblem).

1)

 * </p>

please do me favor - remove that from javadoc, for some kind of understandable reason java8 started to put a violation on close P tag.

Done.
 

2) 

 * Any other return types are disallowed. 

Any other return types will not let method match a setter pattern.

Done.
 

3) 

 * a setter is expanded, so that setter return type can also be a class in

what a bout enums ? in interface there is no members , but enums are kind of the same class , most likely used for Singletons 

Taken care of and test cases added to InputHiddenFiled.java

4) 

/** stack of sets of field names,

* one for each class of a set of nested classes.

*/

private FieldFrame mCurrentFrame; lets refactor that to stack and rename class to ClassFrame.

It is already sort of a stack and since we support setter recognition in the enums as well as classes I think it is better to keep it called FieldFrame.

 

5)  private String mInClassName; as we refactor that to stack , that field will not be required.

Removed.

 

6) 

public void visitToken(DetailAST aAST)

we need to refactor that method to have 

switch() {

VARIABLES:

    processVariable()

CLASS and ENUM:

    processType()

}

too much is put in that methods 

Done.

 

7) 

private void processVariable(DetailAST aAST)

please make a one return, that will simplify method.

I really do not feel that that simplifies the method, but I made changes as you reqested.

 

8) 

private boolean isMethodASetter(DetailAST aMethodAST, String aName)

I like you changes in logic, but please keep one return in method, there is no reason to have two there

Multiple return statements are similiar in nature to use of "break" in "switch case brake" code and thus they really look ok to me, but I did make changes as you requested.

 

9)

private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

please move that method to me located above isMethodASetter, as it is callled first.

Done.

 

 

10)

private static String capitalize(final String aName)

private boolean isIgnoredSetterParam(DetailAST aAST, String aName)

I understand that is not your code , but please do me favor - lets keep one return point from method. 

Again, I do not see how it helps with visual understanding of the code but I have made changes as you have requested.

 

11)

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-aggregate.html

please do me a favor , please activate only your Check , all other should be removed from config, report should be regenerated.

Done. Please see following two files.

    https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-aggregate.html

and

    https://raw.githubusercontent.com/priimak/guava-libraries-17-0/master/checkstyle_google_style.xml

New checkstyle code is here https://github.com/priimak/checkstyle/tree/XHiddenFieldCheck 

in the XHiddenFieldCheck branch. Total chage is in three commits, not squashed.

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmitry,

please recheck that you use latest versions of checkstyle and jxr plugin , links to code does not work properly in your report. did you check validation results yourself ?

thanks,

Roman Ivanov

​

[pri...@gmail.com]

Hi Roman.

Would like me to rebase my changes to the current HEAD of checkstyle?

Dmitri Priimak

From: Roman Ivanov Sent: Tuesday, January 20, 2015 6:14 PM To: Dmitri Priimak Cc: checksty...@googlegroups.com

Subject: Re: hidden field ignore rule for builder setter methods

[Roman Ivanov]

yes, please do, we finally found time to remove old style prefixes from code for members, sorry that might cause a lot of conflicts.

Sorry for caused problems.

--
You received this message because you are subscribed to the Google Groups "checkstyle-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to checkstyle-dev...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Dmitri Priimak]

No problem. Will do it later today.

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmitry,

please always send me link to sources, you are not alone who try to push change to Checkstyle, I can not remember all of you. 

https://github.com/priimak/checkstyle/blob/a037861eeadde38cf25bf5d747474f1e63407783/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

1) 

>         if (!ScopeUtils.inInterfaceOrAnnotationBlock(aAST)

            && ( ScopeUtils.isLocalVariableDef(aAST)

                || (aAST.getType() == TokenTypes.PARAMETER_DEF))) {

            //local variable or parameter. Does it shadow a field?

            final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT);

            final String name = nameAST.getText();

            if ((mCurrentFrame.containsStaticField(name)

please add "\n" , before "//local ...." and " if ((mCurrentFrame.containsStaticField(name)", it is hard to read.

2) 

> private boolean isMethodASetter(DetailAST aMethodAST, String aName)

isSetterMethod. please avoid abbreviation and abreviation-like names, we already have a Check for that, One day I will enforce it over checkstyle .

3) 

> boolean isASetter = false;

the same as above, please rename.

4) 

> private boolean isIgnoredConstructorParam(DetailAST aAST)

> private boolean isIgnoredParamOfAbstractMethod(DetailAST aAST)

I usually ok, so multiple return point from method if method is small, but here you use "!" too much. Please redo that in one return point method.

Example:

     boolean result = false;

        if ((aAST.getType() == TokenTypes.PARAMETER_DEF)

            && mIgnoreConstructorParameter)

        {

        final DetailAST parametersAST = aAST.getParent();

        final DetailAST constructorAST = parametersAST.getParent();

        result = (constructorAST.getType() == TokenTypes.CTOR_DEF);

        }

        return result;

5) 

still waiting from you testing report, code looks ok.

thanks,

Roman Ivanov

​

[Dmitri Priimak]

Hi Roman.

Please take a look at https://github.com/priimak/checkstyle/tree/SimplifiedSetterTest 

which is SimplifiedSetterTest. In that branch I have made changes as you have requested 

(see my comments inlined below) and rebased it on head of the current upsteam master.

On Wed, Jan 21, 2015 at 9:21 AM, Roman Ivanov <romani...@gmail.com> wrote:

Hi Dmitry,

please always send me link to sources, you are not alone who try to push change to Checkstyle, I can not remember all of you. 

https://github.com/priimak/checkstyle/blob/a037861eeadde38cf25bf5d747474f1e63407783/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java

1) 

>         if (!ScopeUtils.inInterfaceOrAnnotationBlock(aAST)

            && ( ScopeUtils.isLocalVariableDef(aAST)

                || (aAST.getType() == TokenTypes.PARAMETER_DEF))) {

            //local variable or parameter. Does it shadow a field?

            final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT);

            final String name = nameAST.getText();

            if ((mCurrentFrame.containsStaticField(name)

please add "\n" , before "//local ...." and " if ((mCurrentFrame.containsStaticField(name)", it is hard to read.

Done

 

2) 

> private boolean isMethodASetter(DetailAST aMethodAST, String aName)

isSetterMethod. please avoid abbreviation and abreviation-like names, we already have a Check for that, One day I will enforce it over checkstyle .

Done.

 

3) 

> boolean isASetter = false;

the same as above, please rename.

Done

 

4) 

> private boolean isIgnoredConstructorParam(DetailAST aAST)

> private boolean isIgnoredParamOfAbstractMethod(DetailAST aAST)

I usually ok, so multiple return point from method if method is small, but here you use "!" too much. Please redo that in one return point method.

Example:

     boolean result = false;

        if ((aAST.getType() == TokenTypes.PARAMETER_DEF)

            && mIgnoreConstructorParameter)

        {

        final DetailAST parametersAST = aAST.getParent();

        final DetailAST constructorAST = parametersAST.getParent();

        result = (constructorAST.getType() == TokenTypes.CTOR_DEF);

        }

        return result;

Done

 

5) 

still waiting from you testing report, code looks ok.

Here are the reports

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-setter-true.html

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/checkstyle-setter-false.html

If you have trouble accessing them, you can try going directly to github at the following URLs

https://github.com/priimak/guava-libraries-17-0/blob/master/target/site/checkstyle-setter-false.html

https://github.com/priimak/guava-libraries-17-0/blob/master/target/site/checkstyle-setter-true.html

Following checktyle config file was used to generate these two reports

https://github.com/priimak/guava-libraries-17-0/blob/master/checkstyle_google_style.xml

Please let me know if you have trouble accessing any of this data.

--

Dmitri Priimak

[Roman Ivanov]

Hi Dmitri,

code is ok.

For report:

0) 

violations 168 vs 163 . So at least in Guava that new option does not help significantly.

1) One of example that builder method is not started from "set...." but it is builder method

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/xref/com/google/common/collect/MinMaxPriorityQueue.html#L174

2) 

Interesting code that is group setter :)

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/xref/com/google/common/collect/testing/AbstractTester.html#L58

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/xref/com/google/common/escape/Escapers.html#L114

3) 

https://rawgit.com/priimak/guava-libraries-17-0/master/target/site/xref/com/google/common/util/concurrent/ThreadFactoryBuilder.html#L132

I would say that is false-positive, but I do not know how to resolve it, algorithm of what is setter should be improved :) one day.

4) This Check is very controversial as it from one side help you to keep names separate , but in the same time does not allow user to enjoy good names in code generation (when IDE propose you to name local variables in user code the same way as they are named in arguments - that is very convenient). But it is problem of person that decide to use that Check :), Check do exactly what it has to :).

Bottom line :

You are ok to DO PR, thanks for reports , they helped a lot.

thanks,

Roman Ivanov

​