Is opened else branch inside a switch-case ok?

Question

My code accidentally ends up with the following structure. Im not sure that is this ok.

switch (msg.type)
{
  case Msg::Type::One:
  case Msg::Type::Two:
    // do nothing
    break;

  case Msg::Type::Open:
    if (msg.isBad())
      break;
    else // opened else branch is ok here??
    // intended fall through

   case Msg::Type::Close:
     goodMsg.push_back(msg);
     doSomethingOther();
     blaBla();
     break;
}

What does "OK" mean? Yes, it compiles. Yes, it does something. The something might or might not be what you want. Either way, it's confusing to a reader. — hobbs, Apr 05 '14 at 22:43
What it is supposed to do? Run next case? If so, you only need `if` to check if you want to break. — Mars, Apr 05 '14 at 22:43
It's compliant code, but I doubt I could call it "OK". See [Duff's Device](http://en.wikipedia.org/wiki/Duff's_device) and we even have an explanation for it [here](http://stackoverflow.com/questions/514118/how-does-duffs-device-work). — Massa, Apr 05 '14 at 22:46
@Mars: intended fall through... But I just realized, I should just delete the else — Industrial-antidepressant, Apr 05 '14 at 22:47
And possibly add a comment explaining the intentional fallthrough :) — keyser, Apr 05 '14 at 22:48

score 4 · Accepted Answer · answered Apr 05 '14 at 22:50

4

It may work, but if you intend to fall through, I would just remove the else altogether, and leave a comment that it is an intentional fall through.

answered Apr 05 '14 at 22:50

László Papp

51,870
39
111
135

Keith Thompson · Answer 2 · 2014-04-18T06:53:50.537

The syntax of an if statement is either:

if (expression) statement

or

if (expression) statement else statement

In your code, the statement associated with the else keyword is ~~the entire switch statement~~ the goodMsg.push_back(msg); (which happens to have a case label associated with it).

I can't tell from your question whether that's what you intended, though your indentation suggests that you meant the switch statement to be independent of the if/else.

If that's what you intended, the else is not useful. Your logic would be more clearly expressed as:

if (condition) break;
case ...

My suggestion: always use curly braces for conditional and loop statements. This lets you avoid having to ask questions like this in the first place:

if (condition) {
    statement
}
else {
    statement
}

An if / else if / ... / else chain is a special case. Syntactically, it's a nested set of if/else statements, but it's convention to treat it as a linear chain, writing:

if (condition) {
    statement
}
else if (condition) {
    statement
}
else {
    statement
}

rather than:

if (condition) {
    statement
}
else {
    if (condition) {
        statement
    }
    else {
        statement
    }
}

Note that I'm using K&R-style brace placement, with each { at the end of a line. Another very common style places both { and } on lines by themselves. Both choices are valid (I obviously have my own preference), as long as you're consistent.

score 1 · Answer 3 · answered Apr 05 '14 at 22:52

I would suggest against it. When it comes to coding, there's a three-point hierarchy of importance that everyone should follow:

Readability
Functionality
Efficiency

I would argue that what you're trying to do here is an attempt at making this more readable by reducing the amount of code written, but the problem is it actually does the opposite.

Try coming up with another way to accomplish what you're trying to accomplish that is easier for someone who is completely foreign to your code to understand.

One suggestion would be to leave off the empty else, and just have the if statement. While this is arguably still not optimal for readability, it's better than with the else statement, and may actually be tolerable in the workplace.

score 1 · Answer 4 · answered Apr 05 '14 at 22:55

Let's mark some areas of your code:

switch (msg.type) {
  case Msg::Type::One:
  case Msg::Type::Two:
    // (1)
    break;

  case Msg::Type::Open:
    if (msg.isBad()) {
      // (2)
      break;
    } else {
      // (3)
      // intended fall through
    }

   case Msg::Type::Close:
     // (4)
     goodMsg.push_back(msg);
     doSomethingOther();
     blaBla();
     break;
}

And here's a table showing what code will be run:

                         (1) (2) (3) (4)
Msg::Type::One            ✔︎
Msg::Type::Two            ✔︎
Msg::Type::Open && Bad        ✔︎       
Msg::Type::Open && !Bad           ✔︎   ✔︎
Msg::Type::Close                      ✔︎

That being said, most readers of this code will assume that the fall through condition is a bug. Therefore, I would strongly recommend refactoring this code.

Is opened else branch inside a switch-case ok?

4 Answers4