New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid boolean parameters for public methods #4709

Closed
romani opened this Issue Jul 12, 2017 · 9 comments

Comments

Projects
3 participants
@romani
Member

romani commented Jul 12, 2017

taken from #4707

classes: ConfigurationLoader , DefaultLogger, XMLLogger

Idea inspection: 'public' method with 'boolean' parameter
From inspection documentation
It's almost always a mistake to add a boolean parameter to a public method (part of an API) if that method is not a setter. When reading code using such a method, it can be difficult to decipher what the boolean stands for without looking at the source or documentation. This problem is also known as the "boolean trap". The boolean parameter can often be profitably replaced with an enum Use the option below to only warn when a method contains more than one boolean parameter.
link for "boolean trap" - https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

It will be good to update but do this accurately to not damage CI that depends on maven plugin that uses such c-tors.

Intellij Idea by visual effect over the code, ease this problem by printing name of parameter before the value:
image
, but I still think that code should be readable without IDE

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

@rnveach , please do second review of the intend to refactor.

Member

romani commented Jul 12, 2017

@rnveach , please do second review of the intend to refactor.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 13, 2017

Member

It will definitely break maven. We would need to coordinate with them first and then our regressioned projects to have them update to the newer version, if it is ever released. Aren't we waiting on maven checkstyle for other changes?
https://github.com/apache/maven-plugins/blob/dd84ee9053fde6f553c163237eb1d0f625f427b7/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java#L377-L378
https://github.com/apache/maven-plugins/blob/dd3acde958cf2470080e859f1a9a3d50b2c488b3/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L566
https://github.com/apache/maven-plugins/blob/dd3acde958cf2470080e859f1a9a3d50b2c488b3/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L562

boolean trap

I understand the thought behind this of why to not allow boolean variables as enumerations would basically self-document, but I'm not sure I would want this kind of refactoring in my own code. I can't imagine many people using those 3 classes you mentioned directly. We can't do checks as they have properties that are true booleans, unless we break almost all our checks and change them too, which it seems like we should as properties are definitely used much more than those classes.

If you want to do this for those classes, then I have no big issue with that.

Member

rnveach commented Jul 13, 2017

It will definitely break maven. We would need to coordinate with them first and then our regressioned projects to have them update to the newer version, if it is ever released. Aren't we waiting on maven checkstyle for other changes?
https://github.com/apache/maven-plugins/blob/dd84ee9053fde6f553c163237eb1d0f625f427b7/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java#L377-L378
https://github.com/apache/maven-plugins/blob/dd3acde958cf2470080e859f1a9a3d50b2c488b3/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L566
https://github.com/apache/maven-plugins/blob/dd3acde958cf2470080e859f1a9a3d50b2c488b3/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L562

boolean trap

I understand the thought behind this of why to not allow boolean variables as enumerations would basically self-document, but I'm not sure I would want this kind of refactoring in my own code. I can't imagine many people using those 3 classes you mentioned directly. We can't do checks as they have properties that are true booleans, unless we break almost all our checks and change them too, which it seems like we should as properties are definitely used much more than those classes.

If you want to do this for those classes, then I have no big issue with that.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 13, 2017

Member

Aren't we waiting on maven checkstyle for other changes?

yes, that is why issue is separate, and will require deprecation process for multiple releases. One day they will upgrade to 8.X version, we can create new constructors now and deprecate existing.
@rnveach , Are you ok with this ?

We can't do checks as they have properties that are true booleans

inspections skips simple setters, please review report in original issue, there are side effects(kind of false-positives), but they are minimal (there will be only 2 suppressions).

Member

romani commented Jul 13, 2017

Aren't we waiting on maven checkstyle for other changes?

yes, that is why issue is separate, and will require deprecation process for multiple releases. One day they will upgrade to 8.X version, we can create new constructors now and deprecate existing.
@rnveach , Are you ok with this ?

We can't do checks as they have properties that are true booleans

inspections skips simple setters, please review report in original issue, there are side effects(kind of false-positives), but they are minimal (there will be only 2 suppressions).

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 13, 2017

Member

inspections skips simple setters, please review report in original issue

We can create our own check if we wish to enforce for checks. Since you wish to do this, it may make sense to do it for the properties too.

Are you ok with this ?

I am fine with that plan.

Member

rnveach commented Jul 13, 2017

inspections skips simple setters, please review report in original issue

We can create our own check if we wish to enforce for checks. Since you wish to do this, it may make sense to do it for the properties too.

Are you ok with this ?

I am fine with that plan.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 13, 2017

Member

We can create our own check if we wish to enforce for checks.

ones we resolve current current GSoC tasks we can put a lot of new Checks to checkstyle, nothing blocks us to do this now, except for lack of time and lack of contributors.

Member

romani commented Jul 13, 2017

We can create our own check if we wish to enforce for checks.

ones we resolve current current GSoC tasks we can put a lot of new Checks to checkstyle, nothing blocks us to do this now, except for lack of time and lack of contributors.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 13, 2017

Member

@Nimfadora , please create new method/ctors for these classes and put deprecation annotation on existing.

Member

romani commented Jul 13, 2017

@Nimfadora , please create new method/ctors for these classes and put deprecation annotation on existing.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 9, 2017

Member

@Nimfadora , last PR is merged, please send PR to activate inspection in config

Member

romani commented Aug 9, 2017

@Nimfadora , last PR is merged, please send PR to activate inspection in config

@romani romani added this to the 8.2 milestone Aug 10, 2017

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Aug 11, 2017

Contributor

@romani I realized that it is already enabled. But two cases are still annotated without deprecation:
DetectorOptions#ignoreCase and IllegalTokenTextCheck#setIgnoreCase

Contributor

Nimfadora commented Aug 11, 2017

@romani I realized that it is already enabled. But two cases are still annotated without deprecation:
DetectorOptions#ignoreCase and IllegalTokenTextCheck#setIgnoreCase

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 11, 2017

Member

But two cases are still annotated without deprecation:
DetectorOptions#ignoreCase and IllegalTokenTextCheck#setIgnoreCase

we can not deprecate this, first if false-positive over Builder patterns, second is Check public property, we can not change them.

inspection is enabled - https://github.com/checkstyle/checkstyle/blob/master/config/intellij-idea-inspections.xml#L199
Issue is closed.

Member

romani commented Aug 11, 2017

But two cases are still annotated without deprecation:
DetectorOptions#ignoreCase and IllegalTokenTextCheck#setIgnoreCase

we can not deprecate this, first if false-positive over Builder patterns, second is Check public property, we can not change them.

inspection is enabled - https://github.com/checkstyle/checkstyle/blob/master/config/intellij-idea-inspections.xml#L199
Issue is closed.

@romani romani closed this Aug 11, 2017

@romani romani moved this from In Progress to Done in Practice What You Preach Aug 11, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment