Too few branches for a switch statement

[grzesiek2010]

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:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[dcbriccetti]

Did you use IDEA’s Refactor -> Inline for this? If so, it requires no review.

[grzesiek2010]

No the last commit as I said I made manually, but it's pretty small.

[dcbriccetti]

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.

[grzesiek2010]

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).

[dcbriccetti]

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.

[lognaturel]

Since master is frozen to anything but bug fixes, we can make a final decision after the freeze is lifted! Thanks for reviewing, @dcbriccetti.

[lognaturel]

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.

[zestyping]

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:

• It is common to write switch statements with only one or two cases, when writing a button click handler (switching on the button ID) or an event handler (switching on an event code or a key code). Developers who write switch statements in this style will be surprised to find that it breaks their build.
• Many of the examples in the official Android documentation use switch statements in this style. Enforcing this rule will mean that even official Android examples won't build.
• Converting switch statements to if statements doesn't significantly improve readability. The result might be slightly better or slightly worse, but the difference is marginal.
• Converting switch statements to if statements makes it harder to add or delete cases, a small to moderate increase in maintenance cost.

In summary, the costs seem quite high and the benefit is low, so I don't think we should enforce this rule.

[lognaturel]

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. 🎉

[dcbriccetti]

In general, I think we should be sparing in adding more static analysis rules

Well said! And yes, @lognaturel, I rejoice.