does this switch statement smell bad?

Question

Switch(some case) {
    case 1:
           // compute something ...
           return something;
           break;
    case 2:
           // compute something ...
           return something;
           break;

/* some more cases ... */

    case X:
           // compute something ...
           return something;
           break;
    default:
           // do something
           return something;
           break;
}

In my opinion:

Assuming this switch statement is justifiable, the return and break just doesnt look right or feel right.

The break is obviously redundant, but is omission poor style (or is this poor style to begin with?) ?

---

I personally dont do this, but there is some of this in the codebase at work. And no, im not going to be self-righteous and correct the codebase.

Answer 1

In my opinion, I would omit the 'break' keyword. I personally think it helps remind people that 'Execution has ended! Nothing more to see here!'.

Answer 2

I would make a small change:

switch(some case) {
    case 1:
           // compute something ...
           break;
    case 2:
           // compute something ...
           break;
/* some more cases ... */
    case X:
           // compute something ...
           break;
    default:
           // do something
           break;
}
return something;

Answer 3

No, omission is not poor style - inclusion is poor style. Those are unreachable statements. Get rid of them.

I like the fact that the cases return directly instead of setting a local variable and then returning just at the bottom - it means that it's incredibly clear when you're reading the code that it does just need to return, and that's all.

Side-note in terms of switching in the first place:

As for whether using a switch statement is the right thing to do here, it really depends on other things. Would it make sense to use a polymorphic type instead? If you're in Java, could you use a smart enum? (You can mimic these in C#, but there isn't as much support.)

I'd say this should at least prompt considering different designs - but it may well be the simplest way to do what you want.

Answer 4

Code smell implies a design problem. This is just an issue of formatting. I think most people would agree that a version with breaks omitted is superior.

Answer 5

The C# Compiler gives a warning if you do this saying that the break is unreachable code. So in my book it is bad form to have both return and break.

Answer 6

The break is redundant.

Some rule of thumb, It is always good to exit a function in one place only, But since this rule was made Try-Catch was invented (oop goto).

If you keep the same style all over your application, I guess you can live with the return in the switch.

Answer 7

I'm not sure how literal that code is intended to be so some of these observations may not be applicable...

"case 1" If you're truly hard-coding numers like this I think it is poor style and you should look into using an enumeration.

If you are simply returning something and there is no additional logic in a subset of the cases, you might consider putting the "somethings" in an array or dictionary and simply addressing them by their index rather than using a switch statement...

return somethings[index]

Answer 8

The code smell here is the hard coded values in the case statements.

As for the returns it is more a question of taste and good judgement. Sure if your case spans mutiple pages it is easier to read if the return is in the case, but then the problem is the big switch to begin with.

Personnaly I prefer the single return option because if ever you need to add some more processing then it is cheaper to refactor than visiting all the cases. Also, if given to someone else to refactor they will not have the temptation to copy paste the code in every case, least I would hope so ... if they do they better hope I am not the one doing their code review (for the copy paste obviously).

Answer 9

Switch statements are themselves a code smell.

Along the lines of what l99057j said, if all you're doing is mapping inputs to constant values, this is a place for an appropriate lookup structure, such as an array/list for sequential inputs or a dictionary/map for a sparse one, not a switch statement. If you're calculating something, then the value would be a delegate that you call with the input.