added hidden field ignore rule for builder setter methods

[wigbam]

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.

[coveralls]

Coverage increased (+0.01%) when pulling 1432d3a on wigbam:master into fc3c0b6 on checkstyle:master.

[coveralls]

Coverage increased (+0.01%) when pulling 396d950 on wigbam:master into fc3c0b6 on checkstyle:master.

[romani]

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)

[coveralls]

Coverage increased (+0.01%) when pulling bd97297 on wigbam:master into fc3c0b6 on checkstyle:master.

[wigbam]

updated the main pull request top comment

[romani]

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]

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]

@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]

@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]

@wigbam, you are right, let me think about it one more time.

[romani]

@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]

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.

[romani]

PR is closed , discussion is moved to mail-list https://groups.google.com/d/msg/checkstyle-devel/aPNYF3RC_yU/lGHsgRPCO38J
After we come to agreement on design , PR could be recreated.