-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Too few branches for a switch statement #2564
Too few branches for a switch statement #2564
Conversation
@@ -214,8 +214,7 @@ public boolean onMenuItemActionCollapse(MenuItem item) { | |||
|
|||
@Override | |||
public boolean onOptionsItemSelected(MenuItem item) { | |||
int i = item.getItemId(); | |||
if (i == R.id.menu_sort) { | |||
if (item.getItemId() == R.id.menu_sort) { |
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.
Did you use IDEA’s Refactor -> Inline for this? If so, it requires no 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.
No the last commit as I said I made manually, but it's pretty small.
|
||
} else if (i == R.id.trace_automatic) { | ||
if (checked) { | ||
} else if (view.getId() == R.id.trace_automatic) { |
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.
One nice thing about a switch (even with only two cases) is you don’t repeat the view.getId()
for each case, and you don’t need a local variable for the value. I’m not sure I like the “too few” rule, but I won’t object until I try to write one.
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.
for each case
sounds too serious here since in the worst case it's for both (only two) cases (switches with three elements are allowed).
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.
I don’t think I agree with the rule—I would find myself quite annoyed if I wrote code where I felt a switch with two cases is best and it was blocked—but I’ll go along with it.
Since |
I have two challenges with enabling this rule. First, switch statements are idiomatic in Android listeners so all sample code will use that pattern. Second, this PR will cause merge conflicts with #2465, #2535 and probably others. At the very least, we should wait until merging #2465. I'm not sure the benefits outweigh the costs, though. Would be great to get more opinions. |
In general, I think we should be sparing in adding more static analysis rules to the set that are enforced, because of the friction they add to the development process. Enforcing a rule like this can impact the develop-build-test feedback loop by breaking the build and requiring extra rounds of editing and building; keeping that iteration loop tight is important for development velocity and quality. So, new rules should meet a fairly high standard of justification; we should only enforce a rule if we think it will generate a large improvement. Let's examine the costs and benefits of this particular rule:
In summary, the costs seem quite high and the benefit is low, so I don't think we should enforce this rule. |
Thanks for that really clear reasoning, @zestyping. I have added your thoughts to some notes I've been keeping around code review process and what criteria we should use to know what to accept and reject. It's currently just loose thoughts here but I intend to share on the forum and invite comments and collaboration soon (hoping for a moment when there aren't so many balls in the air!). Closing this and hoping that @dcbriccetti rejoices. 🎉 |
Well said! And yes, @lognaturel, I rejoice. |
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
it's just a pmd rule we should follow.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Generall, all switches have been replaced automatically using IntelliJ studio but changes in the last commit I made manually so it should be reviewed.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.