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

reevaluate tokens in checkstyle config for NeedBraces #3735

Closed
rnveach opened this Issue Jan 18, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jan 18, 2017

Taken from PR #3723 ,

When reviewing the tokens of NeedBraces, it is unclear if we should add the token LAMBDA to our configuration. Our style demand braces in all case for IF, FOR, etc. so it seems LAMBDA should follow this rule too.

New configuration item created for this token:

    <module name="NeedBraces">
      <property name="tokens" value="LAMBDA"/>
      <property name="allowSingleLineStatement" value="true"/>
    </module>

When testing this out and fixing violations from adding the token, TeamCity failed with violations like:

Statement lambda can be replaced with expression lambda (7) 
EqualsHashCodeCheck.java (1)
162: finishTree() Statement lambda can be replaced with expression lambda

for code like:

-            .entrySet().stream().filter(detailASTDetailASTEntry ->
-                objBlockWithHashCode.remove(detailASTDetailASTEntry.getKey()) == null)
-            .forEach(detailASTDetailASTEntry -> {
+            .entrySet().stream().filter(detailASTDetailASTEntry -> {
+                return objBlockWithHashCode.remove(detailASTDetailASTEntry.getKey()) == null;
+            }).forEach(detailASTDetailASTEntry -> {
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 18, 2017

Member

My opinion, I don't really use lambdas or a lot of Java 7+ features, but I think I like adding braces to lambdas just like we do for other constructs, especially if it spans multiple lines. It makes it easier to understand where the start and end is, especially when it is nested inside other method calls. It also makes it easier to see what is in the lambda and what isn't.
The reason why add braces to ifs and such is in-case more code is added to the if, the git difference won't show adding the new braces and make the change easier to understand. It may be unlikely that we add more statements to a lambda, but it still helps with the same purpose.

Member

rnveach commented Jan 18, 2017

My opinion, I don't really use lambdas or a lot of Java 7+ features, but I think I like adding braces to lambdas just like we do for other constructs, especially if it spans multiple lines. It makes it easier to understand where the start and end is, especially when it is nested inside other method calls. It also makes it easier to see what is in the lambda and what isn't.
The reason why add braces to ifs and such is in-case more code is added to the if, the git difference won't show adding the new braces and make the change easier to understand. It may be unlikely that we add more statements to a lambda, but it still helps with the same purpose.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 18, 2017

Member

Looks like the intend is to remove braces if possible -
https://blog.jetbrains.com/idea/2014/09/the-inspection-connection-issue-1/#crayon-587e989b37764467122309
but our style demand braces in all case for IF, FOR,..... so LAMBDA should follow this rule too, so Inspections set need to be updated.

Member

romani commented Jan 18, 2017

Looks like the intend is to remove braces if possible -
https://blog.jetbrains.com/idea/2014/09/the-inspection-connection-issue-1/#crayon-587e989b37764467122309
but our style demand braces in all case for IF, FOR,..... so LAMBDA should follow this rule too, so Inspections set need to be updated.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 16, 2017

Member

fix is merged.

Member

romani commented Feb 16, 2017

fix is merged.

@romani romani closed this Feb 16, 2017

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