-
Notifications
You must be signed in to change notification settings - Fork 86
Unwrap else after return in preceding if
#645
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
Conversation
} | ||
|
||
@Override | ||
public TreeVisitor<?, ExecutionContext> getVisitor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual recipe that needs review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test case I'd add is with nested ifs in the primary branch. Something like:
if (someCondition) {
// default logic
if (somethingRare) {
return "terminate all processing";
}
} else {
// non-standard logic
}
IMHO no changes should be made here.
if ("one".equals(str)) { | ||
return 1; | ||
} else if ("two".equals(str)) { | ||
return 2; | ||
} else if ("three".equals(str)) { | ||
return 3; | ||
} else { | ||
return Integer.MAX_VALUE; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interesting to add a test case where only some of the if
s have the return
here. Like only the two
branch. That's similar to ifWithoutReturnNotChanged
test case, but with chaining.
Also, not sure if it should go into the same recipe, or a separate one. The same kind of analysis and changes could be made with
|
Added a new recipe, polished it up, and ran it on this repository to flush out any potential issues. Seemed straightforward enough and nicely reduces needless indentation. Let me know if you agree and then we can run this more broadly, and enforce it going forward.