Switch-if antipattern



  •  Here's a fun one that I just ran into (my first actual "WTF" contribution iirc). It's PHP, anonymized to protect the innocent:

    switch($product) {
    case 123:
    case 124:
    case 125:
    if ($product == 123) {
    header("Location: urlblah123");
    } elseif ($product == 124) {
    header("Location: urlblah124");
    } elseif ($product == 125) {
    header("Location: urlblah125");
    }
    exit;
    }

    Is the "switch-if" antipattern a thing yet? :)



  • I wouldn't call that an antipattern, I'd just call that retarded programming from someone who doesn't understand basic logic. Antipatterns, though ugly, actually work. Your example just has a bunch of unreachable code in it.



  •  That looks like a C style fall-through switch to me, so the example should work. Even though it's still retarded.

    I can see one legitimate reason for the pattern: If some (but not all) of the cases are almost identical with only little differences. Something like this: 

     switch (status) {
    case 1:
    case 2:
    doCommonStuff();
    if (status == 1) doSpecialStuffFor1();
    doMoreCommonStuff();
    if (status == 2) doSpecialStuffFor2();
    doCommonCleanup();
    break;
    case 3:
    doSomethingCompletelyDifferent();
    break;
    //... 20 other cases follow...
    }
    But even that would be a maintenance nightmare.


  • @mott555 said:

    I wouldn't call that an antipattern, I'd just call that retarded programming from someone who doesn't understand basic logic. Antipatterns, though ugly, actually work. Your example just has a bunch of unreachable code in it.

    Depending on the language, that probably isn't unreachable code. Given the lack of a break, all three of those cases will fall through to the if-elseif block of code for case 125. It's almost as if someone decided to use the switch for validation, but still use ifs for logic.

    Or maybe someone looked at the if/ifelse block, told the programmer to use a switch statement, and this is the hideousness that ensued. I can't think of any valid reason for it.


  • 🚽 Regular

    @mott555 said:

    I wouldn't call that an antipattern, I'd just call that retarded programming from someone who doesn't understand basic logic. Antipatterns, though ugly, actually work. Your example just has a bunch of unreachable code in it.

    Read the code again. It's reachable code. It's simply the worst abuse of a switch-case I've ever seen.



  • I'm used to C# which requires you to put "break" at the end of each case, otherwise you get a compiler error. Being able to hit multiple cases with one run through a switch statement just isn't something I'd even consider.

    But if $product contains the value 123, why would it enter the case for 125 to even get to the if for 123? Unless there is code inside the case for 123 that might change the value to 125.



  • @mott555 said:

    I'm used to C# which requires you to put "break" at the end of each case, otherwise you get a compiler error. Being able to hit multiple cases with one run through a switch statement just isn't something I'd even consider.

     

    That's just retarded. Does it forbid the use of an "or" (whatever the actual syntax) in a "if" statement as well?

    Edit: just saw on the MSDN website that they encourage the use of goto for fall through. Yay!



  • @Zemm said:

    @mott555 said:

    I'm used to C# which requires you to put "break" at the end of each case, otherwise you get a compiler error. Being able to hit multiple cases with one run through a switch statement just isn't something I'd even consider.

     

    That's just retarded. Does it forbid the use of an "or" (whatever the actual syntax) in a "if" statement as well?

    Edit: just saw on the MSDN website that they encourage the use of goto for fall through. Yay!

     

    If I wanted to fall through I'd just use a bunch of normal "if" statements. Switch-case is already what I would consider a slightly-confusing feature, at least to new or inexperienced programmers.

    [quote user="From Eric Lippert's blog"]The purpose of this rule, and of the no-fall-through rule in general, is that we want you to be able to arbitrarily re-order your switch sections without accidentally introducing a breaking change.[/quote]

    The goto thing is retarded. Goto is a reserved keyword but I didn't realize it actually did anything.



  • @mott555 said:

    I'm used to C# which requires you to put "break" at the end of each case, otherwise you get a compiler error. Being able to hit multiple cases with one run through a switch statement just isn't something I'd even consider.
     

    C# requires the break after you are doing something in a case, but you can still stack them like in the example.  The requiring a break is to prevent accidental fall through (a relatively common bug iirc).



  • @mott555 said:

    If I wanted to fall through I'd just use a bunch of normal "if" statements. Switch-case is already what I would consider a slightly-confusing feature, at least to new or inexperienced programmers.
     

    switch() allows you to chain OR conditions on values in a far more readable manner than in an if.

    I recommend you use it.



  • @dhromed said:

    @mott555 said:

    If I wanted to fall through I'd just use a bunch of normal "if" statements. Switch-case is already what I would consider a slightly-confusing feature, at least to new or inexperienced programmers.
     

    switch() allows you to chain OR conditions on values in a far more readable manner than in an if.

    I recommend you use it.

    I agree that if you [i]must[/i] chain multiple OR conditions, switch statements are the way to go.  However, whenever I have to chain multiple OR conditions, I start thinking "there must be a better way", ie. polymorphism, look-up tables, etc.



  • @EJ_ said:

     Here's a fun one that I just ran into (my first actual "WTF" contribution iirc). It's PHP, anonymized to protect the innocent:

    switch($product) {
    case 123:
    case 124:
    case 125:
    if ($product == 123) {
    header("Location: urlblah123");
    } elseif ($product == 124) {
    header("Location: urlblah124");
    } elseif ($product == 125) {
    header("Location: urlblah125");
    }
    exit;
    }

    Is the "switch-if" antipattern a thing yet? :)

    I'd call it the Superfluous-Switch antipattern.



  • @locallunatic said:

    The requiring a break is to prevent accidental fall through (a relatively common bug iirc).

    Yes. There are cases where you wanted to use it but it caused more problems than it solved. Purist programmers may not like that C# holds your hand here a bit

    switch case (product) 
    {
        case 123:
            package(product, ProductClass.Flammable);
        case 124:
            package(product, ProductClass.Breakable);
        case 125:
            package(product, ProductClass.None);
    }
    

    C thinks this is fine which usually results in runtime errors. C# will complain about breaks.

     Edit: I now see that PSWorx said it earlier and better than me.



  • @locallunatic said:

    @mott555 said:

    I'm used to C# which requires you to put "break" at the end of each case, otherwise you get a compiler error. Being able to hit multiple cases with one run through a switch statement just isn't something I'd even consider.
     

    C# requires the break after you are doing something in a case, but you can still stack them like in the example.  The requiring a break is to prevent accidental fall through (a relatively common bug iirc).

     

     Yep.

     Works in C#:

    switch(num){
        case 0:
        case 1:
        case 2:
        default:
            doSomething();
        break;
    }

    Compiler error in C#:

     switch(num){
        case 0:
            doSomething();
        case 1:
            doSomething();
        case 2:
            doSomething();
        default:
            doSomething();
    }



  • As the case labels show where each new block starts, does this mean that the only purpose of the "break" keyword in a C# switch is to distinguish your first example from the following?

    switch(num){
        case 0:
        case 1:
        case 2:
        break;
        default:
            doSomething();
        break;
    }

     

    And other than that, it's just meaningless cruft needed to shut the compiler up? Nice.



  • @__moz said:

    As the case labels show where each new block starts, does this mean that the only purpose of the "break" keyword in a C# switch is to distinguish your first example from the following?

    Yes. Which is extremely useful if you have more complex/bigger cases. It's so easy to forget the break.



  •  ... and then, of course, there is Duff's device...



  • @PSWorx said:

     ... and then, of course, there is Duff's device...

    Heh.  I got to write this recently:

    -  entry->name = xstrdup (p);
    + switch (sym_style)
    + {
    + case ss_win32:
    + if (p[0] == '@')
    + {
    + /* cf. Duff's device. */
    + case ss_none:
    + entry->name = xstrdup (p);
    + break;
    + }
    + /* FALL-THROUGH. */
    + case ss_uscore:
    + entry->name = concat ("_", p, NULL);
    + break;
    + default:
    + check (false, LDPL_FATAL, "invalid symbol style requested");
    + break;
    + }

    Uses both fall-through and Duff's device(*) at the same time :-)

     

    (*) - Alright, it's not quite exactly Duff's, but it still interleaves a switch with a block-based control flow structure, so close enough I thought. 



  •  @b-redeker said:



    @__moz said:


    As the case labels show where each new block starts, does this mean that the only purpose of the "break" keyword in a C# switch is to distinguish your first example from the following?



    Yes. Which is extremely useful if you have more complex/bigger cases. It's so easy to forget the break.

    I know that it is easy to do for some people. I don't know that you should be punished for doing it, though.

    C's default "end of case" behaviour makes some sense as "go to the next instruction" is a reasonable thing to do at the end of a case, and C has no explicit command for this.

    If a language has a "fallthrough" command, or forbids that sort of behaviour altogether, its designers are free to choose any default they want. Were I designing such a language, I could choose "go to the first statement after the switch", as it's usually what [i]I[/i] want. Microsoft chose "refuse to compile", which never is.



  • @__moz said:

    If a language has a "fallthrough" command, or forbids that sort of behaviour altogether, its designers are free to choose any default they want. Were I designing such a language, I could choose "go to the first statement after the switch", as it's usually what I want. Microsoft chose "refuse to compile", which never is.

    You know they don't just randomly do things like this. C# is designed to be used by human beings, not cyborg space robots from the future. The requirement is there because it prevents a few classes of extremely common bugs that human beings commonly make.



  • Problem:

    case 1:

        doSomething(); //for case 1

    case 2:

    case 3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Leaving out the break in case 2 leads to bugs and is extremely easy to do.



    Sane(?) solution:

    case 1:

        doSomething(); //for case 1

        continue; //or 'fallthrough' or some other such statement

    case 2:

    case 3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Leaving out the break in case 2 is a syntax error.



    Microsoft's solution:

    case 1:

        doSomething(); //for case 1

        goto _case3;

    case 2:

    case 3:

    _case3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Which is just ew.



    And I should have known CS would manage to fuck up line break formatting even in code tags. Seems to be no way to preserve indentation either. (oh but hey it magically came back) Wheee.



  • @lolwtf said:

    Problem:

    case 1:

        doSomething(); //for case 1

    case 2:

    case 3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Leaving out the break in case 2 leads to bugs and is extremely easy to do.



    Sane(?) solution:

    case 1:

        doSomething(); //for case 1

        continue; //or 'fallthrough' or some other such statement

    case 2:

    case 3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Leaving out the break in case 2 is a syntax error.



    Microsoft's solution:

    case 1:

        doSomething(); //for case 1

        goto _case3;

    case 2:

    case 3:

    _case3:

        doMoreThings(); //for cases 1, 2 and 3

        break;

    case 4:

        doUnrelatedThing(); //for case 4

        break;


    Which is just ew.



    And I should have known CS would manage to fuck up line break formatting even in code tags. Seems to be no way to preserve indentation either. (oh but hey it magically came back) Wheee.

    Orrrr, do it properly:

    case 1:
    case 2:
    case 3:
        if(value == 1) doSomething(); //for case 1
        doMoreThings(); //for cases 1, 2 and 3
        break;
    case 4:
        doUnrelatedThing(); //for case 4
        break;
    

    That said, goto gets a bad rap. We're not programming in BASIC-A any more, we don't use line numbers, and we can tell where a label branches. Sure, it can be abused like anything else, but I regularly use goto in situations like this:

    again:
    foreach(Thing t in ListOfThings) {
        if(t.Blah == foo) {
            doImportantWork();
            SomeCollection.Add(t.Bar());
        }
        if(t.Something == 234) {
            //shit, we need to fix this, which invalidates all the work we've done.
            t.Fix();
            undoImportantWork();
            SomeCollection.Clear();
            goto again;
        }
    }
    

    Actually, that example is probably more complex than how I usually do it. But, the alternative is to use a while(true) loop which implies that we [i]need[/i] to loop more than once, rather than it being an exceptional case.



  •  If you don't indent the break;, you don't forget it.

    case "a":
       statement;
    break;
    case "b":
       statement;
    break;
    case "c":
       statement;
    break;

     

     




  • And if you have many conditions inside a case then you can always nest switches.

    switch (value) {
        case 1:
        case 2:
        case 3:
        case 4:
            switch (value) {
                case 1:
                case 2:
                    switch (value) {
                        case 1:
                            doIt(); //for case 1
                            break;
                    }
                    doSomething(); //for cases 1 and 2
                    break;
            }
            doMoreThings(); //for cases 1, 2, 3 and 4
            break;
        case 5:
            doUnrelatedThing(); //for case 5
            break;
    }
    


  •  See? Cases are almost like mini nukes.



  • @PSWorx said:

     ... and then, of course, there is Duff's device...

     

     I did that quite often when I wrote in assembler, but I didn't realise it would work in C...

     



  • @dhromed said:

     See? Cases are almost like mini nukes.

     

    Duff's Device, then, is the multiple-nuke-firing Fat Man that you get from the basement of that National Guard base.



  • @Someone You Know said:

    Duff's Device, then, is the multiple-nuke-firing Fat Man that you get from the basement of that National Guard base.
     

    Wasteful and unpredictable.



  •  @blakeyrat said:

    @__moz said:
    If a language has a "fallthrough" command, or forbids that sort of behaviour altogether, its designers are free to choose any default they want. Were I designing such a language, I could choose "go to the first statement after the switch", as it's usually what I want. Microsoft chose "refuse to compile", which never is.
    You know they don't just randomly do things like this. C# is designed to be used by human beings, not cyborg space robots from the future. The requirement is there because it prevents a few classes of extremely common bugs that human beings commonly make.

    Do you have any insight into why they may have made the decision above, or is your post just a reaction to the physical pain criticism of Microsoft appears to cause you? I ask because you haven't actually mentioned the change I suggested in your response.

    I can't quite imagine the following exchange:
    Designer: We're thinking of making each "case" block in a "switch" statement separate, so that it only runs a single block for each value instead of falling through to the next one whenever you don't use a "break" statement.
    Focus group: Please do; we'd really appreciate it as we often forget "break" statements and have never heard of "lint".
    Designer: We could make it so that each "case" acts as if it was preceded by a C-style "break".
    Focus group: How would we make one block run for several values of the variable then?
    Designer: Okay, we could treat those as a special case. The "cases" which follow statements (things ending with semi-colons) end the previous block. The ones which follow "case" labels (things ending with colons) do not.
    Focus group: How would we remember the difference between the two situations? We like your special case, but we'd like them to have different syntax to remind us that it behaves differently.
    <The designer suggests various innovative things, which are all rejected.>
    Designer: How about requiring every normal "case" block to end with a "break" statement.
    Focus group: That would be wonderful. There is one other thing, though. There are a few things we sometimes put at the end of a block to prevent the computer from reaching the end of a block. We'd like the compiler to accept these without a "break".
    Designer: Make a list; I'm going out for a cigarette.



  • @dhromed said:

    @Someone You Know said:

    Duff's Device, then, is the multiple-nuke-firing Fat Man that you get from the basement of that National Guard base.
     

    Wasteful Glorious and unpredictable awesome.

     

    FTFY.



  • @Someone You Know said:

    @dhromed said:

    @Someone You Know said:

    Duff's Device, then, is the multiple-nuke-firing Fat Man that you get from the basement of that National Guard base.
     

    Wasteful Glorious and unpredictable awesome.

     

    FTFY.

     

    not mutually exlusive!



  •  my autotagger knows me better than I do.



  • @dhromed said:

     my autotagger knows me better than I do.

    you're obsessed with large smurfs?



  • @HighlyPaidContractor said:

    @dhromed said:

     my autotagger knows me better than I do.

    you're obsessed with large smurfs?

    large sub-pixel smurfs.



  •  @HighlyPaidContractor said:

    @dhromed said:

     my autotagger knows me better than I do.

    you're obsessed with large smurfs?

    Or smurf subpixels and large personal obsessions



  • @lolwtf said:

    Microsoft's solution:

    ...

    Just goto case 3;



  • @__moz said:

     @blakeyrat said:

    @__moz said:
    If a language has a "fallthrough" command, or forbids that sort of behaviour altogether, its designers are free to choose any default they want. Were I designing such a language, I could choose "go to the first statement after the switch", as it's usually what I want. Microsoft chose "refuse to compile", which never is.
    You know they don't just randomly do things like this. C# is designed to be used by human beings, not cyborg space robots from the future. The requirement is there because it prevents a few classes of extremely common bugs that human beings commonly make.

    Do you have any insight into why they may have made the decision above, or is your post just a reaction to the physical pain criticism of Microsoft appears to cause you? I ask because you haven't actually mentioned the change I suggested in your response.

    I can't quite imagine the following exchange:
    Designer: We're thinking of making each "case" block in a "switch" statement separate, so that it only runs a single block for each value instead of falling through to the next one whenever you don't use a "break" statement.
    Focus group: Please do; we'd really appreciate it as we often forget "break" statements and have never heard of "lint".

    Guys, we already have a switch that can't fall through between cases.  It's called an if..else ladder.




  • @__moz said:

     @blakeyrat said:

    @__moz said:
    If a language has a "fallthrough" command, or forbids that sort of behaviour altogether, its designers are free to choose any default they want. Were I designing such a language, I could choose "go to the first statement after the switch", as it's usually what I want. Microsoft chose "refuse to compile", which never is.
    You know they don't just randomly do things like this. C# is designed to be used by human beings, not cyborg space robots from the future. The requirement is there because it prevents a few classes of extremely common bugs that human beings commonly make.

    Do you have any insight into why they may have made the decision above, or is your post just a reaction to the physical pain criticism of Microsoft appears to cause you? I ask because you haven't actually mentioned the change I suggested in your response.

    He just wrote it - falling through is commonly a bug that is missed by coders, and C# is a language that says "if I'm not 100% clear on what you're asking, I'm going to make you be clear" instead of guessing what you want and getting it wrong half the time.  For the "go to the first statement after the switch"... a lot of times it's NOT really what you want - well, not what OTHER coders want, anyway.  They forgot to put the break statement in there.  If this is C, you have a bug, but in C#, the compiler says "hey, did you really want to fallthrough?"  If you did, you put it in there, and it's perfectly easy.  If you didn't, you don't have to deal with tracking down a bug, you just put your break statement in there.

     

    Try this conversation instead:

    C# creator 1: What do we do with switches and breaks?

    C# creator 2: Well, we can require them or we can not.  What are the pros and cons?

    1: If we require them, the compiler will catch that common bug for the user, and if they want fallthrough they can say where to go from there with a goto.  This also allows them to reorder at will without breaking anything.

    2: And the pros of fallthrough are... it will make moz less angry.

    1: That sounds like a con.

    2: Yeah, no fallthroughs it is.


Log in to reply