3

Is it okay to combine cases that share assignments and repeat the case for assignments that are not shared, or is it preferred to just keep each separated?

To illustrate with a simple example.. case 0 and 180 both include w = 330 so they have been combined; the value assigned to x is different for each so they are repeated to do the x assignment.

switch(window.orientation) {
    case   0:
    case 180:
        w = 330;
        //break
    case   0:
        x = '-180px';
        //break
    case -90:
    case  90:
        w = 480;
        x = '0';
        break;
    case 180:
        x = '-80px';
        break;
}
davidcondrey
  • 34,416
  • 17
  • 114
  • 136
  • I wouldn't combine since case 0 and case 180 have different x values (might possibly if the branches were identical). I would avoid combining cases due to difficult readability/later maintainability alone. – Will Sep 24 '14 at 21:10

4 Answers4

6

When it comes to do more than one operation per element in a switch statement, it's always better to not repeat the same case twice. You can easily achieve this by summing up all the fragments of code that are under the same case.

For example, if you want to perform operation A on case 0 and operation B on case 0 and case 1 then you should do something like this:

switch(variable) {
    case 0:
        // operation A;
    case 1:
        // operation B;
        break;
}

This will execute both operation A and B on case 0, because there's no break on case 0.

Now let's assume you write something like this:

switch(variable) {
    case 1:
        x = 1;
        break;
    case 1:
        x = 2;
        break;
}

The above code will end up assigning the value 1 to the variable x. The second case 1, saying x = 2 will never be reached, because of the break statement in the first case 1.

So if you have got to perform different operations on case 0 and case 1, but they share some operation, that's better to separate the cases repeating some lines of code instead of writing case 1 twice, because this makes your code easier to read and slightly faster.

So in your code, the best way to achieve what you want is this one:

switch(window.orientation) {
    case   0:
        x = '-180px';
        w = 330;
        break;
    case 180:
        x = '-80px';
        w = 330;
        break;
    case -90:
    case  90:
        w = 480;
        x = '0';
        break;
}
Marco Bonelli
  • 63,369
  • 21
  • 118
  • 128
1

I would say don't combine - too high of a chance of making a mistake...like you did. The 2nd case 180 will never be reached, since the first one has a break; after it.

If everything is the same, I think it's ok to combine though (like the -90, 90).

Gerrat
  • 28,863
  • 9
  • 73
  • 101
1

If you run static analysis tool like Coverity it report minor error if you combine multiple switch cases like below.

   case -90:
   // Intentionally fall through
   case  90:
        w = 480;
        x = '0';
        break;

Better to repeat some lines of code than to make some undesired error.

       case -90:
            w = 480;
            x = '0';
            break;
       case  90:
            w = 480;
            x = '0';
            break;
Deepu
  • 598
  • 6
  • 12
-1

It can't work. As soon as a break instruction is executed, the remaining code in the switch block is not executed. You could fix it like this:

switch(window.orientation) {
    case   0:
    case 180:
        w = 330;
    case   0:
        x = '-180px';
        break;
    case 180:
        x = '-80px';
        break;
    case -90:
    case  90:
        w = 480;
        x = '0';
        break;
    }
Pascal Le Merrer
  • 5,883
  • 20
  • 35