Monday, March 28, 2011

What is the best way to replace or substitute if..else if..else trees in programs?

This question is motivated by something I've lately started to see a bit too often, the if..else if..else structure. While it's simple and has its uses, something about it keeps telling me again and again that it could be substituted with something that's more fine-grained, elegant and just generally easier to keep up-to-date.

To be as specific as possible, this is what I mean:

if (i == 1) {
    doOne();
} else if (i == 2) {
    doTwo();
} else if (i == 3) {
    doThree();
} else {
    doNone();
}

I can think of two simple ways to rewrite that, either by ternary (which is just another way of writing the same structure):

(i == 1) ? doOne() : 
(i == 2) ? doTwo() :
(i == 3) ? doThree() : doNone();

or using Map (in Java and I think in C# too) or Dictionary or any other K/V structure like this:

public interface IFunctor() {
    void call();
}

public class OneFunctor implemets IFunctor() {
    void call() {
     ref.doOne();
    }
}

/* etc. */    

Map<Integer, IFunctor> methods = new HashMap<Integer, IFunctor>();
methods.put(1, new OneFunctor());
methods.put(2, new TwoFunctor());
methods.put(3, new ThreeFunctor());
/* .. */
(methods.get(i) != null) ? methods.get(i).call() : doNone();

In fact the Map method above is what I ended up doing last time but now I can't stop thinking that there has to be better alternatives in general for this exact issue.

So, which other -and most likely better- ways to replace the if..else if..else are out there and which one is your favorite?

Your thoughts below this line!


Okay, here are your thoughts:

First, most popular answer was switch statement, like so:

switch (i) {
    case 1:  doOne(); break;
    case 2:  doTwo(); break;
    case 3:  doThree(); break;
    default: doNone(); break;
}

That only works for values which can be used in switches, which at least in Java is quite a limiting a factor. Acceptable for simple cases though, naturally.

The other and perhaps a bit fancier way you seem to sugges is to do it using polymorphism. The Youtube lecture linked by CMS is an excellent watch, go see it here: "The Clean Code Talks -- Inheritance, Polymorphism, & Testing" As far as I understood, this would translate to something like this:

public interface Doer {
    void do();
}

public class OneDoer implements Doer {
    public void do() {
     doOne();
    }
}
/* etc. */

/* some method of dependency injection like Factory: */
public class DoerFactory() {
    public static Doer getDoer(int i) {
     switch (i) {
      case 1: return new OneDoer();
      case 2: return new TwoDoer();
      case 3: return new ThreeDoer();
      default: return new NoneDoer();
     }
    }
}

/* in actual code */

Doer operation = DoerFactory.getDoer(i);
operation.do();

Two interesting points from the Google talk:

  • Use Null Objects instead of returning nulls (and please throw only Runtime Exceptions)
  • Try to write a small project without if:s.

Also in addition one post worth mentioning in my opinion is CDR who provided his perverse habits with us and while not recommended to use, it's just very interesting to look at.

Thank you all for the answers (so far), I think I might have learned something today!

From stackoverflow
  • Use a switch/case it's cleaner :p

  • The switch statement of course, much prettier then all those if's and else's.

  • In this simple case you could use a switch.

    Otherwise a table-based approach looks fine, it would be my second choice whenever the conditions are regular enough to make it applicable, especially when the number of cases is large.

    Polymorphism would be an option if there are not too many cases, and conditions and behaviour are irregular.

  • These constructs can often be replaced by polymorphism. This will give you shorter and less brittle code.

    vava : not me but I don't think code will be shorter :)
    WolfmanDragon : polymorphism is great, when it can be used. How about inside of loops? I have never seen it used there. can it be done?
    Brian Rasmussen : @Vadim - not necessarily, but if the if/switch blocks are repeated it probably will.
    Brian Rasmussen : @WolfmanDragon - sure it can. It is just calling a method based on the actual runtime type.
    WolfmanDragon : I'm going over and asking this as a question then, I don't see it.
    mouviciel : I downvoted. This guy is fed up with a construct as complex as `if ... else if ... else if ...` and the suggested solution is polymorphism? Sorry, it doesn't sound right for me.
    Brian Rasmussen : @mouviciel, please check out the google tech talk on polymorphism for additional input.
    mouviciel : Excuse my previous rough comment, I thought that in the given example a switch were the simplest solution (as suggested in http://stackoverflow.com/questions/519515 which is the follow-up of this question). Anyway, I will follow your advice and take a look at this google tech talk.
    David Thornley : If the difference is worth subclassing about, polymorphism is the answer. Otherwise, I don't think it's worth worrying about. If trees are only harmful when they're bushy, not when they're long and straggly.
    Brian Rasmussen : @David - I didn't say that polymorphism is the answer to every possible problem. It's not a silver bullet.
    Pavel Feldman : While I also do think that polymorphism can be the right solution here, this comment could be more specific and give some examples. I'm too lazy, so just upvoting :)
    Pavel Feldman : If in one place you use switch(i) { case 1:doOne();break; case 2:doTwo();break; } and in another place switch(i){ case 1:doFirst(); case 2:doSecond() } then it clearly means polymorphism is a must.
  • A switch statement:

    switch(i)
    {
      case 1:
        doOne();
        break;
    
      case 2:
        doTwo();
        break;
    
      case 3:
        doThree();
        break;
    
      default:
        doNone();
        break;
    }
    
  • In Object Oriented languages, it's common to use polymorphism to replace if's.

    I liked this Google Clean Code Talk that covers the subject:

    The Clean Code Talks -- Inheritance, Polymorphism, & Testing

    ABSTRACT

    Is your code full of if statements? Switch statements? Do you have the same switch statement in various places? When you make changes do you find yourself making the same change to the same if/switch in several places? Did you ever forget one?

    This talk will discuss approaches to using Object Oriented techniques to remove many of those conditionals. The result is cleaner, tighter, better designed code that's easier to test, understand and maintain.

  • There's two parts to that question.

    How to dispatch based on a value? Use a switch statement. It displays your intent most clearly.

    When to dispatch based on a value? Only at one place per value: create a polymorphic object that knows how to provide the expected behavior for the value.

  • Outside of using a switch statement, which can be faster, none. If Else is clear and easy to read. having to look things up in a map obfuscates things. Why make code harder to read?

  • switch (i) {
      case 1:  doOne(); break;
      case 2:  doTwo(); break;
      case 3:  doThree(); break;
      default: doNone(); break;
    }
    

    Having typed this, I must say that there is not that much wrong with your if statement. Like Einstein said: "Make it as simple as possible, but no simpler".

  • In OO paradigm you could do it using good old polymorphism. Too big if - else structures or switch constructs are sometimes considered a smell in the code.

  • Depending on the type of thing you are if..else'ing, consider creating a hierarchy of objects and using polymorphism. Like so:

    class iBase
    {
       virtual void Foo() = 0;
    };
    
    class SpecialCase1 : public iBase
    {
       void Foo () {do your magic here}
    };
    
    class SpecialCase2 : public iBase
    {
       void Foo () {do other magic here}
    };
    

    Then in your code just call p->Foo() and the right thing will happen.

  • switch statement or classes with virtual functions as fancy solution. Or array of pointers to functions. It's all depends on how complex conditions are, sometimes there's no way around those if's. And again, creating series of classes to avoid one switch statement is clearly wrong, code should be as simple as possible (but not simpler)

  • Naturally, this question is language-dependent, but a switch statement might be a better option in many cases. A good C or C++ compiler will be able to generate a jump table, which will be significantly faster for large sets of cases.

  • The Map method is about the best there is. It lets you encapsulate the statements and breaks things up quite nicely. Polymorphism can complement it, but its goals are slightly different. It also introduces unnecessary class trees.

    Switches have the drawback of missing break statements and fall through, and really encourage not breaking the problem into smaller pieces.

    That being said: A small tree of if..else's is fine (in fact, i argued in favor for days about have 3 if..elses instead of using Map recently). Its when you start to put more complicated logic in them that it becomes a problem due to maintainability and readability.

  • If you really must have a bunch of if tests and want to do different things whenwver a test is true I would recommend a while loop with only ifs- no else. Each if does a test an calls a method then breaks out of the loop. No else there's nothing worse than a bunch of stacked if/else/if/else etc.

  • I would go so far as to say that no program should ever use else. If you do you are asking for trouble. You should never assume if it's not an X it must be a Y. Your tests should test for each individually and fail following such tests.

    mghie : This is far too strong, as can be seen in the question - "else doNone()" may of course be the right thing to do, instead of enumerating all other possible values.
  • In python, I would write your code as:

    actions = {
               1: doOne,
               2: doTwo,
               3: doThree,
              }
    actions[i]()
    
  • I use the following short hand just for fun! Don't try anyof these if code clearity concerns you more than the number of chars typed.

    For cases where doX() always returns true.

    i==1 && doOne() || i==2 && doTwo() || i==3 && doThree()
    

    Ofcourse I try to ensure most void functions return 1 simply to ensure that these short hands are possible.

    You can also provide assignments.

    i==1 && (ret=1) || i==2 && (ret=2) || i==3 && (ret=3)
    

    Like instad of writting

    if(a==2 && b==3 && c==4){
        doSomething();
    else{
        doOtherThings();
    }
    

    Write

    a==2 && b==3 && c==4 && doSomething() || doOtherThings();
    

    And in cases, where not sure what the function will return, add an ||1 :-)

    a==2 && b==3 && c==4 && (doSomething()||1) || doOtherThings();
    

    I still find it faster to type than using all those if-else and it sure scares all new noobs out. Imagine a full page of statement like this with 5 levels of indenting.

    "if" is rare in some of my codes and I have given it the name "if-less programming" :-)

    Esko : This is just perverse :) +1 for that alone.
  • I regard these if-elseif-... constructs as "keyword noise". While it may be clear what it does, it is lacking in conciseness; I regard conciseness as an important part of readability. Most languages provide something like a switch statement. Building a map is a way to get something similar in languages that do not have such, but it certainly feels like a workaround, and there is a bit of overhead (a switch statement translates to some simple compare operations and conditional jumps, but a map first is built in memory, then queried and only then the compare and jump takes place).

    In Common Lisp, there are two switch constructs built in, cond and case. cond allows arbitrary conditionals, while case only tests for equality, but is more concise.

    (cond ((= i 1)
           (do-one))
          ((= i 2)
           (do-two))
          ((= i 3)
           (do-three))
          (t
           (do-none)))

    (case i (1 (do-one)) (2 (do-two)) (3 (do-three)) (otherwise (do-none)))

    Of course, you could make your own case-like macro for your needs.

    In Perl, you can use the for statement, optionally with an arbitrary label (here: SWITCH):

    SWITCH: for ($i) {
        /1/ && do { do_one; last SWITCH; };
        /2/ && do { do_two; last SWITCH; };
        /3/ && do { do_three; last SWITCH; };
        do_none; };
    
  • The example given in the question is trivial enough to work with a simple switch. The problem comes when the if-elses are nested deeper and deeper. They are no longer "clear or easy to read," (as someone else argued) and adding new code or fixing bugs in them becomes more and more difficult and harder to be sure about because you might not end up where you expected if the logic is complex.

    I've seen this happen lots of times (switches nested 4 levels deep and hundreds of lines long--impossible to maintain), especially inside of factory classes that are trying to do too much for too many different unrelated types.

    If the values you're comparing against are not meaningless integers, but some kind of unique identifier (i.e. using enums as a poor man's polymorphism), then you want to use classes to solve the problem. If they really are just numeric values, then I would rather use separate functions to replace the contents of the if and else blocks, and not design some kind of artificial class hierarchy to represent them. In the end that can result in messier code than the original spaghetti.

0 comments:

Post a Comment