Code smell in this switch statement?

Question

I'm wondering where a switch statement of this style should be changed to an if else statement.

switch (foo) // foo is an enumerated type
{
    case barOne:
        if (blahOne)
        {
            DoFunction(//parameters specific to barOne);
            break;
        }
   case barTwo:
        if (blahTwo)
        {
            DoFunction(//parameters specific to barTwo);
            break;
        }
   //etc.
   default:
       // Whatever happens if none of the case's conditionals are met
}

Basically fall through is happening unless a condition is met for one of the cases. The cases are very similar, differing only in what needs to be checked for and what needs to be passed, which is why I used a switch statement.

Would it be better to use if else if? Otherwise, is it clear enough to stay, but unclear enough to warrant a comment about the fallthrough? Polymorphism is also always an option, but it seems like overkill to me.

Answer 1

Always fancy if/else K&R referenced so avoid switch to a. enable more advanced test and b. avoid forgetting default and c. improve readability

Answer 2

Seems like it would make more sense to compute the parameters within if/else or switch logic, store them in variables, and then call the function afterwords with whatever parameters you computed.

Answer 3

It seem the switch is more readable than if/else if, especially when there are more options. Rule of thumb: If you have to do more than three comparisons against the same data then use switch.

Answer 4

I would prefer a set of if/else-if statements, but after heavy development and growth, both the switch and the if/else-if will start to rot. Imagine what would happen if the number of types defined in the foo enum grew large; both the switch and if/else-if strategy will not look pretty.

At some point, you will need to either abstract the DoSomething function (think Strategy pattern) or abstract the parameters to the DoSomething function (put the foo enum in DoSomething's prototype or make the parameter to DoSomething be a sibling type to a base class).

Answer 5

It seems like this might do strange things in some cases. What if foo == bar1, and blahOne is false, but blahTwo is true? Then you'll fall through and call the function under the foo == bar2 case, even though foo doesn't equal bar2.

That might be unexpected in practice, but if it ever did occur it might be tough to debug. I'd vote for an if else in this case, because the flow is simpler.

if (foo == barOne && blahOne)
{
    DoFunction(/*parameters specific to barOne*/);
}
else if (foo == barTwo && blahTwo)
{
    DoFunction(/*parameters specific to barTwo*/);
}
else
{
    // Handle the fallthrough case.
}

Of course, if the intention is that blahTwo can be evaluated even though foo != barTwo, then the switch might be the best way to do it, but I'd definitely be in favor of some explanatory comments in that case.

Answer 6

I don't really see a code smell here. The code snippet is very readable. Depending on what blahOne and blahTwo represent and what the code does, you might also be able to enhance readability by baking them into the first enum. So instead of two cases with one if statement each, you get four cases.

Answer 7

If I were you I would do:

if (foo == barOne && blahOne) {
  DoFunction(/* params specific to barOne */);
} else if (foo == barTwo && blahTwo) {
  DoFunction(/* params specific to barTwo */);
} else {
  DoDefaultStuff();
}

That seems a lot more readable to me. You have too much extra logic to warrant a simple switch/case. Also, if you have what essentially amounts to an if nested in an if (and that's pretty much it)... you should consider using &&

Answer 8

I actually quite like it the way it is now; the switch is acting as a short circuit to skip the evaluation of blahOne etc based on the enum value. The only thing I'd suggest is a comment at the end of each case noting the fall-through, as that's often missed on a casual glance. And probably one at the top saying why it's like this.

I'm assuming, of course, that this is in fact the intended behaviour; if it's not, then as others have said, it might be just plain wrong.

Answer 9

Wow, I'm not sure whether I would call that a "code smell" or just a plain bug. I look at that code and immediately think, "that can't be right".

Switch case fall-through should always be clearly documented in a code comment, with justification if it's obscure.

I was disappointed 15 years ago when Java came out and I saw that they hadn't fixed the default-fallthrough behaviour of C's switch statement. However, Go has fixed it, with an explicit fallthrough statement required if that's what you want:

In a case or default clause, the last statement only may be a "fallthrough" statement (§Fallthrough statement) to indicate that control should flow from the end of this clause to the first statement of the next clause. Otherwise control flows to the end of the "switch" statement.

Answer 10

A misplaced break sends sends the control to fall through to if (blahTwo) when case barOne is true but case barTwo is false.

case barOne:
    if (blahOne)
    {
        DoFunction(//parameters specific to barOne);
        // DONT'T BREAK HERE
    }
    break;
    // BREAK HERE INSTEAD

Answer 11

Your code is definitely smelly as posted because the flow of control can contradict the comments as follows;

If foo is barOne and blahOne is false and blahTwo is true then DoFunction() is called with parameters specific to barTwo. But foo is barOne not barTwo.

Maybe what you really meant was the following, no fall throughs and no smells in my opinion.

switch (foo) // foo is an enumerated type
{
    case barOne:
        if (blahOne)
            DoFunction(//parameters specific to barOne);
        break;
   case barTwo:
        if (blahTwo)
            DoFunction(//parameters specific to barTwo);
        break;
   //etc.
   default:
       // Whatever happens if none of the case's conditionals are met
}