1

I'm doing a code review at the moment on Java Android code. I am looking at a function where a programmer has decided to use a switch statement to wrap a single condition.

I am considering making a comment that this should be an if block instead. What are your thoughts, what would you do if you saw this in code?

It looks like a case where the switch will always be the one case and never expand beyond that ( because of the codes surrounding context ). Would the correct words be that this is the wrong construct for this use case?

pseudo code of function:

trackAnalyticsEvent()

  switch
    case is device not supported
      analytics->trackEvent(device type not supported)
      // No break or return

note: The core analytics code is actually handled elsewhere in the project. This a very specific single case inside a class with a specific focused usage.

Improve this question
5
  • 3
    How is the switch being used? There are cases (no pun intended) where the use of a switch is idiomatic in Android development even when only one case is present, eg checking the identifiers passed back to methods that handle events like dialog results.
    – Jules
    Commented Mar 16, 2017 at 18:51
  • The switch is being used in a function written to purely decide whether or not to track something to Google Analytics. If the case is true the analytic event is tracked.
    – Dave Thomas
    Commented Mar 16, 2017 at 18:52
  • 2
    This seems like an overkill. I would change it to an if.
    – Andy
    Commented Mar 16, 2017 at 19:00
  • 1
    I agree with David, but I would ask first to the programmer, Why he/she thinks that the switch is the appropiated choice here.
    – Laiv
    Commented Mar 16, 2017 at 21:52
  • I ended up going with asking the intent of the programmer. It is always more polite in code review, to ask when things seem unclear. This ended up being the best choice for my way to give feedback.
    – Dave Thomas
    Commented Mar 17, 2017 at 2:33

6 Answers 6

Reset to default
2

The fact that the function is called trackAnalyticsEvent seems to me like it will do more than just check. In any case (pun intended) a case would not be relevant since it would just be a simple if-else scenario.

Improve this answer
1
  • Good play with the words!
    – Dave Thomas
    Commented Mar 17, 2017 at 2:35
4

There are situations where today I have one case, but I know I'm going to add more cases tomorrow.

There are situations where many functions follow the same pattern, some ending with a dozen cases, some ending with one case.

Both are situations where I would use a case statement.

Improve this answer
4
  • 1
    "There are situations where today I have one case, but I know I'm going to add more cases tomorrow" - In those situations I'd still prefer to use the if statement and change it to switch statement only after adding more cases. If by some reason you change your mind, and there won't be more cases added, there is a chance that the single case switch statement will remain in the code and will confuse someone in the future.
    – kmaczuga
    Commented Mar 17, 2017 at 10:05
  • 2
    How would anyone be confused by a single case switch statement? And why on earth would I write code that I know I need to change soon, which is extra work and extra chances of introducing a bug?
    – gnasher729
    Commented Mar 17, 2017 at 20:33
  • @gnasher729, The question you're answering exists because someone got confused by a single case switch statement. For every line of code you write, someone reading it will always wonder why it was written that way.
    – chris
    Commented Mar 17, 2017 at 22:05
  • 1
    The OP didn't get confused. He wanted to know whether a switch should be replaced with an if (another question is whether you should ask for it in a code review). He fully understood the code. "Wondering why it is written that way" is not being confused.
    – gnasher729
    Commented Mar 17, 2017 at 22:44
1

If this is an intermediate update in a larger task, and he has a comment (with a date) saying that more cases will be added later when such-and-such is ready, then I would say it is OK. For example, if you plan to support several interchangable hardware devices and he has only written the code for the first one thus far, then that is fine.

But if there are not immediate plans for additional cases, he should use an If block instead.

Also, if this is an edit to existing code, there may have been several cases previously and he condensed/refactored them away. In such a situation, he did well by consolidating the code, but overlooked the fact that he should change the Switch to an If when there is only a single case remaining - which would be an easy oversight to make. I would leave a comment like "don't forget that, after removing the other cases, you can also simplify this into an If statement". This has the connotation that he is on the right track, so it is more encouraging.

Improve this answer
1
  • Your first paragraph is a bad approach. If that's how the code is being designed, he should adopt the Strategy Design Pattern and not change the working code in the future.
    – Bruno Brant
    Commented Mar 17, 2017 at 22:49
1

A switch statement and a chain of ifs result in the same machine-level instructions. You choose one or the other based on code clarity.

You have zero gains on expressiveness (and I one may argue that you have loss) if you create a switch with a single case.

You also have zero gains on coding speed in the case where you know that "in the future" there are going to be more cases, since it will not take any more than a few seconds to turn your if into a switch.

Some other answers tell us that a switch signals that "this could grow". That simply goes against SOLID, as it does not respect Open-Closed Principle, because if that code is ready, the only way to alter it's behavior is to change it.

Improve this answer
2
  • It's worth noting that OCP is probably the most arguable of all the SOLID principles, and that there are types of code where it is routinely ignored. It may work well for framework code and business logic, but in user interface code (eg low level event handlers) and low level operating system interface code it is probably best ignored, because following it in these situations often leads to a massive increase in complexity for little benefit. My suspicion is that the module containing the code in question would fall in the latter category
    – Jules
    Commented Mar 18, 2017 at 16:46
  • @Jules, indeed, strictly following OCP may lead to an increase in complexity that is not justified. While I'll not guess whether it's justifiable or not to apply it in this case, I concede that a finite, predictable list of conditions is better expressed as a simple switch clause. I may remove it from the answer if it will feel more correct. However, I still disagree with "it may grow in the future", since that at least ignores YAGNI.
    – Bruno Brant
    Commented Mar 20, 2017 at 15:14
0

I'd say it's a matter of context.

I'm perfectly content to see a switch, or an if.

But, if I see a switch, I'll think this could grow.

If I see an if, I won't be thinking this could grow.

Thus: if it's something that may grow, it's perfectly fine as a switch (in fact, I'd prefer that). If it's something that won't, I'd suggest changing it to an if.

At the very least, ask about the intent behind the single case switch.

As an aside, I would not expect even the most novice developer to be confused by a single case switch: I'd use it as a good lesson on how to write (and read) informative code by referring to the could it grow? train of thought.

Improve this answer
0

Had the same case myself in a code review, and I would say it depends on the context. In order to evaluate the case, consider the following: Will the condition produce more cases in the future or its return is simply one value? If the result is only one, then no need for a switch. Otherwise, consider it. In your case, the check is clearly producing a true or false result so I would go with the if statement.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.