Switch statement with returns — code correctness

问题:

Let's say I have code in C with approximately this structure:

switch (something)
{
    case 0:
      return "blah";
      break;

    case 1:
    case 4:
      return "foo";
      break;

    case 2:
    case 3:
      return "bar";
      break;

    default:
      return "foobar";
      break;
}

Now obviously, the "break"s are not necessary for the code to run correctly, but it sort of looks like bad practice if I don't put them there to me.

What do you think? Is it fine to remove them? Or would you keep them for increased "correctness"?

回答1:

Remove the break statements. They aren't needed and perhaps some compilers will issue "unreachable code" warnings.

回答2:

I would take a different tack entirely. Don't RETURN in the middle of the method/function. Instead, just put the return value in a local variable and send it at the end.

Personally, I find the following to be more readable:

String result = "";

switch (something) {
case 0:
  result = "blah";
  break;
case 1:
  result = "foo";
  break;
}

return result;

回答3:

Personally I would remove the returns and keep the breaks. I would use the switch statement to assign a value to a variable. Then return that variable after the switch statement.

Though this is an arguable point I've always felt that good design and encapsulation means one way in and one way out. It is much easier to guarantee the logic and you don't accidentally miss cleanup code based on the cyclomatic complexity of your function.

One exception: Returning early is okay if a bad parameter is detected at the beginning of a function--before any resources are acquired.

回答4:

Keep the breaks - you're less likely to run into trouble if/when you edit the code later if the breaks are already in place.

Having said that, it's considered by many (including me) to be bad practice to return from the middle of a function. Ideally a function should have one entry point and one exit point.

回答5:

Remove them. It's idiomatic to return from case statements, and it's "unreachable code" noise otherwise.

回答6:

I would remove them. In my book, dead code like that should be considered errors because it makes you do a double-take and ask yourself "How would I ever execute that line?"

回答7:

I'd normally write the code without them. IMO, dead code tends to indicate sloppiness and/or lack of understanding.

Of course, I'd also consider something like:

char const *rets[] = {"blah", "foo", "bar"};

return rets[something];

Edit: even with the edited post, this general idea can work fine:

char const *rets[] = { "blah", "foo", "bar", "bar", "foo"};

if ((unsigned)something < 5)
    return rets[something]
return "foobar";

At some point, especially if the input values are sparse (e.g., 1, 100, 1000 and 10000), you want a sparse array instead. You can implement that as either a tree or a map reasonably well (though, of course, a switch still works in this case as well).

回答8:

I would say remove them and define a default: branch.

回答9:

Wouldn't it be better to have an array with

arr[0] = "blah"
arr[1] = "foo"
arr[2] = "bar"

and do return arr[something];?

If it's about the practice in general, you should keep the break statements in the switch. In the event that you don't need return statements in the future, it lessens the chance it will fall through to the next case.

回答10:

For "correctness", single entry, single exit blocks are a good idea. At least they were when I did my computer science degree. So I would probably declare a variable, assign to it in the switch and return once at the end of the function

回答11:

What do you think? Is it fine to remove them? Or would you keep them for increased "correctness"?

It is fine to remove them. Using return is exactly the scenario where break should not be used.

回答12:

Interesting. The consensus from most of these answers seems to be that the redundant break statement is unnecessary clutter. On the other hand, I read the break statement in a switch as the 'closing' of a case. case blocks that don't end in a break tend to jump out at me as potential fall though bugs.

I know that that's not how it is when there's a return instead of a break, but that's how my eyes 'read' the case blocks in a switch, so I personally would prefer that each case be paired with a break. But many compilers do complain about the break after a return being superfluous/unreachable, and apparently I seem to be in the minority anyway.

So get rid of the break following a return.

NB: all of this is ignoring whether violating the single entry/exit rule is a good idea or not. As far as that goes, I have an opinion that unfortunately changes depending on the circumstances...

回答13:

I say remove them. If your code is so unreadable that you need to stick breaks in there 'to be on the safe side', you should reconsider your coding style :)

Also I've always prefered not to mix breaks and returns in the switch statement, but rather stick with one of them.

回答14:

I personally tend to lose the breaks. Possibly one source of this habit is from programming window procedures for Windows apps:

LRESULT WindowProc (HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
    switch (uMsg)
    {
        case WM_SIZE:
            return sizeHandler (...);
        case WM_DESTROY:
            return destroyHandler (...);
        ...
    }

    return DefWindowProc(hwnd, uMsg, wParam, lParam);
}

I personally find this approach a lot simpler, succinct and flexible than declaring a return variable set by each handler, then returning it at the end. Given this approach, the breaks are redundant and therefore should go - they serve no useful purpose (syntactically or IMO visually) and only bloat the code.

回答15:

I think the *break*s are there for a purpose. It is to keep the 'ideology' of programming alive. If we are to just 'program' our code without logical coherence perhaps it would be readable to you now, but try tomorrow. Try explaining it to your boss. Try running it on Windows 3030.

Bleah, the idea is very simple:

---

Switch ( Algorithm )
{

 case 1:
 {
   Call_911;
   Jump;
 }**break**;
 case 2:
 {
   Call Samantha_28;
   Forget;
 }**break**;
 case 3:
 {
   Call it_a_day;
 }**break**;

Return thinkAboutIt?1:return 0;

void Samantha_28(int oBed)
{
   LONG way_from_right;
   SHORT Forget_is_my_job;
   LONG JMP_is_for_assembly;
   LONG assembly_I_work_for_cops;

   BOOL allOfTheAbove;

   int Elligence_says_anyways_thinkAboutIt_**break**_if_we_code_like_this_we_d_be_monkeys;

}
// Sometimes Programming is supposed to convey the meaning and the essence of the task at hand. It is // there to serve a purpose and to keep it alive. While you are not looking, your program is doing  // its thing. Do you trust it?
// This is how you can...
// ----------
// **Break**; Please, take a **Break**;

/* Just a minor question though. How much coffee have you had while reading the above? I.T. Breaks the system sometimes */

回答16:

Exit code at one point. That provides better readability to code. Adding return statements (Multiple exits) in between will make debugging difficult .

回答17:

If you have "lookup" type of code, you could package the switch-case clause in a method by itself.

I have a few of these in a "hobby" system I'm developing for fun:

private int basePerCapitaIncomeRaw(int tl) {
    switch (tl) {
        case 0:     return 7500;
        case 1:     return 7800;
        case 2:     return 8100;
        case 3:     return 8400;
        case 4:     return 9600;
        case 5:     return 13000;
        case 6:     return 19000;
        case 7:     return 25000;
        case 8:     return 31000;
        case 9:     return 43000;
        case 10:    return 67000;
        case 11:    return 97000;
        default:    return 130000;
    }
}

(Yep. That's GURPS space...)

I agree with others that you should in most cases avoid more than one return in a method, and I do recognize that this one might have been better implemented as an array or something else. I just found switch-case-return to be a pretty easy match to a lookup table with a 1-1 correlation between input and output, like the above thing (role-playing games are full of them, I am sure they exist in other "businesses" as well) :D

On the other hand, if the case-clause is more complex, or something happens after the switch-statement, I wouldn't recommend using return in it, but rather set a variable in the switch, end it with a break, and return the value of the variable in the end.

(On the ... third? hand... you can always refactor a switch into its own method... I doubt it will have an effect on performance, and it wouldn't surprise me if modern compilers could even recognize it as something that could be inlined...)