Writing less code: The "else" statement


Source code is written once; and read over and over again. So make sure it is easy to read and understand.


I keep seeing a poor coding practice. I see it in production code, in test code, in X++ code, in C# code, in C++ code, in examples on the web, when interviewing candidates. I see it where ever I look. It is a practice that adds unnecessary complexity to source code, it make the source code harder to read, and harder to write. And what disturbs me the most is, how easy it is to avoid.


Take a look at this simple code:


boolean isNegative(int value)
{
    if (value<0)
    {
        return true; 
    }
    else
    {
        return false;
    }
}  
The "else" statement is completely superfluous. This rewrite does exactly the same, and is easier to read:
boolean isNegative(int value)
{
    if (value<0)
    {
        return true; 
    }

    return false;   
}
Yes; the compiler will optimize the "else" statement away - but that is not my point. The compiler can easily see through the weirdest constructs. I (and most other human beings) cannot. Granted; every developer worth his salt should have no problem understanding both of the above implementations. However often the requirements are more complex.
int foo(int bar)
{
    if ( /*expr1*/ )
    {
        throw /*some exception*/; 
    }
    else
    {
        if ( /*expr2*/ ) 
        {
            return 1;
        }
        else
        {
            if ( /*expr3*/ ) 
            {
                return 2;
            }
            else
            {
                if ( /*expr4*/ ) 
                {
                    return 3;
                }                
                else
                {
                    throw /*some exception*/;
                }
            }
       }
    }
}
Or, this slightly better version: 
int foo(int bar)
{
    if ( /*expr1*/ )
    {
        throw /*some exception*/; 
    }
    else if ( /*expr2*/ ) 
    {
        return 1;
    }
    else if ( /*expr3*/ ) 
    {
        return 2;
    }
    else if ( /*expr4*/ ) 
    {
        return 3;
    }
    else
    {
        throw /*some exception*/;
    }
}
Could be simply rewritten as:
int foo(int bar)
{
    if ( /*expr1*/ )
    {
        throw /*some exception*/; 
    }
    if ( /*expr2*/ ) 
    {
        return 1;
    }
    if ( /*expr3*/ ) 
    {
        return 2;
    }
    if ( /*expr4*/ ) 
    {
        return 3;
    }      
    throw /*some exception*/;
}

Never-ever-under-any-circumstances-for-any-reason-what-so-ever write an "else" statement if the preceding block unconditionally returns or throws an exception.

Now I got it off my chest 🙂

Comments (17)

  1. Andrew Hock says:

    I would agree with the commenter who stated that the lack of implementation of a single point of return largely negates your example. You’re using poor coding practices to explain poor coding proctices.

    I realize you acknowledge this flaw, but I hope you see my point…

  2. s3b says:

    This is inefficient in terms of CPU usage… The idea that you’d want to do this, particularly in C#, makes me want to cry. I know, I know, C# was introduced to eliminate the need to write in language such as C and ASM (apparently)… but look, in that example foo will NEVER end up being “boo”, and think of how it works, just briefly in C:

    // foo = “boo”;

    foo = malloc(4);

    *foo = ‘b’;

    *(foo+1) = ‘o’;

    *(foo+2) = ‘o’;

    *(foo+3) = ‘’;

    // if (true) {

    if (true) {

    // foo = “bar”;

    *foo = ‘b’;

    *(foo+1) = ‘a’;

    *(foo+2) = ‘r’;

    *(foo+3) = ‘’;

    }

    Ignoring the call to the function malloc(), this code will go through at least 7 instructions… then it has another 2, then another 7.

    if (true)

    {

      foo = “bar”;

    }

    else

    {

      foo = “boo”

    }

    This code will end up performing 9 instructions at the most. It is almost twice as fast.

    <quote>

    I also like avoiding elses when setting string values. e.g.

    if (true)

    {

      foo = “bar”;

    }

    else

    {

      foo = “boo”

    }

    becomes

    foo = “boo”;

    if (true)

    {

      foo = “bar”;

    }

    </quote>

  3. Nesting brackets is a bugaboo of mine.  When I see three or four nested brackets I wanna vomit.  

    I keep the nested brackets to a minimum whenever possible, and live by the rule of thumb that code which must introduce a third nested bracket should be refactored into a supporting method.  

  4. dewetha: Thank you for your comments.

    I fully agree that focus should be on *good* code and not *minimal* code.

    Besides all the traits you list, good code also reduces complexity.

    I believe the superflous else-statements adds complexity.

  5. dewetha says:

    I agree with your final statement but I believe the focus should be on good code. Using the minimal code and best code are not always the same thing IMO. I see it as good code is easy to read, easy to debug, easy to extend/modify and above all protects the data that is being processed. After all who wants a processing routine that mangles the data and is tough to debug.

    Your first example is simplistic but is not really all that wrong. The following examples, you were correct in saying they are terrible. Your first example allows a simple refactoring of the code on the false side of the equation. But that is splitting hairs on such a simple process.

    All of that, in turn must be balanced against budgets and deadlines. After all, we don’t like to work for free :).

    I like the following code(to a degree but that is another discussion) because it is extensible. IMO, anything other than if/ else decision should be a case statement, But I digress. The following structure actually created more code but is easier to debug and subclass IMO. It requires the creation of 2 more methods.

    int foo(int bar)

        int retValue;
    ;

        if(this.Validate(int)); /* throw excpetion for values out of range */
        {
            switch( int)
            {
                case /*expr2*/:
                    retValue = 1;
                    break;

                case /*expr3*/
                    .
                    .
                    .

                default:
                    retValue  = this.HandleOtherInts(int); // this deal with unhandled ints
            }
        }
        return retValue;
    }

    The two new methods can throw errors or return a proper value. This would allow me to extend this class, and override the “HanndleOtherInts” method to add new functionality later on with having to worry about breaking any code and retesting the old code at all.

  6. Jaredpar: I put loads of consideration into the argument you are making before writing my blog post. I ended up convencing myself that even 5 years down the road (or after a “decade of decaying” as I like to call it); I think that I’m better off not cluttering the flow of the code by trying to anticipate future changes.

    What convenced me is that if a block (Block A below) no longer unconditionally returns; then Block C will be executed. The code in Block C will in most cases have to be updated to accomodate this change in flow.

    if (/* Something is true */)
    {
       //Block A
       return;
    }
    else
    {
       //Block B
    }
    //Block C

    I think you are better off collapsing Block B and C – and then update the code as/if needed.

  7. Thanks for all the comments. Great to see people are reading my blog.

    I’m also in favor of the one-entry-one-exit. I do not see that as the opposite advice. Actually my advice is a step in this direction instead of strewing the code with return statemens. I advocate ending the method with a return or throw statement – instead of having the return statements nested.

    What I’m advocating against is a return-statement followed by an else-statement.

    The examples I’ve used are conceived and not real-life – mostly to stress my point.

    Often the constructs I’m advocating against are implementations of parameter validations. Here my point is even more important, as:

    1. The block (containing the return statement) rarely grows over the years.

    2. The following code is much easier to read, if it is not indented off the screen, by nested-if-else-statements. And it is typically this code that is interesting.

    I had a candidate writing this code to me during an interview:

    int factorial(int value)
    {
       if (value < 0)
       {
           throw new exception(…)
       }
       else 
       {
           if (value == 0)
           { 
               return 1;
           }
           else
           {
               if (value == 1)
               {
                   return 1;
               }
               else
               {
                   int result = 1;
                   for (i=1; i <= value; i++)
                       result *= i;
                   return result;
               }
           }
       }
    }

    I find it hard to see that all paths return a value, and I find that the interesting code (the loop) is buried deep inside some trivial input validations.

    John S: Depending on your language, you want to be carefully on making too many string assignments.

  8. I’m in favor of keeping the else statements.  When the code is originally authored the intent is straight forward.  5 years from now the body if the various blocks will grow, someone will insert a block of code that does not unconditionally return and suddenly the rest of your if’s will be executing in a condition which wasn’t possible when you originally wrote your code.  

  9. ErikW says:

    I’d argue that all of your examples are poor programming style.  One of the first things drilled into me in school was "One Entrance, One Exit", and while exceptions toss a monkey wrench in that, I’ve always believed you should only ever have one return statement in a function.

    One of my biggest beefs with most languages is that their decision and control structures are too rigid.  It’s hard to get out of a deeply nested structure without using extraneous for-loop breaks or goto’s (or worse, using exceptions as gotos).

    I’m not sure what the proper solution is, but it’s not the status quo.

  10. Pavel says:

    The opposite advice would be to never ever use "return" other than before the closing brace of the function, since using it elsewhere is essentially equivalent to goto (the same goes for break/continue, by the way).

    Anyway, people who tend to lean towards functional style often prefer explicit "else" there, just out of habit (and because it does look clearer to them).

  11. Compiler keywords for primitive types? Why?

  12. I prefer the usage of "else if" as it tells the reader that only one of the two paths is executed, without even looking at the code in-between.

    Because the "return" of "throw" typically is the last statement in before the next "if", the difference is not huge. But I absolutely do not agree that this his a hard and fast rule, which must be followed "Never-ever-under-any-circumstances-for-any-reason-what-so-ever".

  13. Wow, I never knew people had a problem with this..

    Although I can see the point with the first and trivial example, I find it easier to visualise the branching in the more complex example when using the "else if" or even the fully expanded version. Actually, it makes way more sense to me to use else blocks, isn’t it generally quite rare to have multiple if statements and every possible outcome returns or throws? Because as soon as there is a path that doesn’t, it gets way harder to read when don’t have the expanded version. Obviously this is all opinion, maybe it comes down to how different people’s brains process this sort of thing, but I’m afraid you’re all weirdos! 🙂

    John S: Seriously.. I can’t get my head around why you prefer that! I’d just keep the if/else and lose the {}.. Even better, how’s this?

    foo = true ? "bar" : "boo";

  14. John S. says:

    I also like avoiding elses when setting string values. e.g.

    if (true)
    {
       foo = “bar”;
    }
    else
    {
       foo = “boo”
    }

    becomes

    foo = “boo”;
    if (true)
    {
       foo = “bar”;
    }

  15. Coming soon, the elseif boolean command…

  16. You’ve been kicked (a good thing) – Trackback from DotNetKicks.com

  17. Julian says:

    The first example is IMNHO better written as:

    boolean isNegative(int value)
    {
       return value<0;
    }  

Skip to main content