Friday, December 2, 2011

Single-Return, AKA: Elephant-proofing your bathtub

Single-entry, single-exit programming was a great breakthrough that led to a serious improvement in the lives of many programmers. In the jurassic period of software development, anyway. In the modern era, among good programmers, I think that single-exit is an example of the many once-best practices that outlive their usefulness.

Method length and nesting are our primary enemies. Multiple returns in nested, long methods turn trouble into disaster for the poor developer who has to understand and debug a routine, not to mention the guy who lost the coin toss and has to actually modify it.  Once code is bad, multiple returns makes it far worse.

Start with pure function length:
  • In a 500-line function two returns is dangerous, and five would be maddening.
  • Drop to 50 lines, and it's still pretty daunting.
  • Drop to 10 lines, and the risk of missing a return or misunderstanding the method is slight.
  • At 5 lines, there's no hint of the problem that single-exit solves.
Switch to nesting depth:
  •  If you are nested 15 conditionals and loops deep, that extra return is obscure with a capital O. 
  • At 5 conditionals deep it's frightful. It's hard to tell at a glance which lines are being executed and when.
  • But what if you're never more than two levels deep in a function? Returns are crazy obvious.
See how this one rule serves you supremely well when your code stinks, but does you no favors when your code is short and clear? 
Q: "Doctor, I broke my arm in two places! What should I do?"
A: "Stay out of those places!"
Single-entry/single-exit can even be harmful to the readability of an otherwise short function if it forces variables to be created up front instead of at first use, or if it causes code to be more deeply nested. 

     int doSomething(int parameter) {
        int result = 0;
        if ( 0 != parameter ) {
           result = x/parameter;
        }
       return result
     }

Here the real feature of the function (the division) is hidden inside an if clause. It makes the code seem as if its goal is to return zero, and that the division is a side effect that occurs when a condition is just right. It'a s funny de-emphasis of the essential goal of the function.

Let's invert the condition to create a guard clause instead:

   int doSomething(int parameter) {
       if ( 0 == parameter ) {
           return 0;
       }
       return x/parameter;
   }

The actual functionality is exalted, and the early return is more clearly an effect of having a parameter that you must protect against. The readability here is not degraded, but instead enhanced.  This is a good time to remind the reader that the shortness of the function makes it so.

It is no worse even if I squish an if onto one line (another no-no in many shops). 
     int doSomething(parameter) {
       if ( 0 == parameter ) {return 0;}
       return x/parameter;
     }

The same rule seems to apply when there are more parameters to check and when the work to be done is more than a single line:


     int doSomething(int parameter1, double parameter2) {
       if ( 0 == parameter1 ) {
           return 0;
       }
       if ( (parameter2 < 0) || (parameter2 > 1)) {
           return 0;
       }
       double part1 = parameter2 * x;
       int result = int(part1/parameter);
       return result;
     }


We have the advantage that the function is divided into checking parameters and returning results, so it is still pretty readable and clean. There is little nesting, and few lines. On the other hand, the astute reader will notice that we're up to 11 lines now, and with a little more expansion we could have a Long Method on our hands, at which time the single-exit rule will apply again (welcome back to the Jurassic period).

We will outgrow an awful lot of our hard-n-fast rules by writing small, clear functions. A lot of "best practices" are really just "ways to tolerate poor practices."  

Some of our old 1970s C programming rules were very valid in their time, but following them when they don't apply is as silly as a Chicagoan elephant-proofing his bathtub.  

6 comments:

  1. A slight adjustment, which I think is a little better.

    int doSomething(int parameter) {
    if (parameter == 0) [
    return 0;
    } else {
    return x / parameter;
    }
    }

    This variant becomes explicit that there are two return values and you will reach one of them at the same level. By having them at separate indentation levels it is a little less apparent and possibly prone to misunderstanding.

    Over all I agree with the gist of your posting.

    James here brought up a good point yesterday, about best practices. Basically we need to be constantly evaluating our best practices to make sure they still are the best.

    https://plus.google.com/105719943141175185993/posts/FVT9MngLmW4

    ReplyDelete
  2. What about the practice of 'Yoda checking'

    if (0 == parameter) {
    return 0;
    }

    ReplyDelete
  3. @Aaron - true enough while it's one line. If the actual work is five to twenty lines then having it wrapped in the ELSE is annoying. Don't tell UncleBobMartin I suggested that I might have a 20-line function. He'll revoke my green band.

    ReplyDelete
  4. @LancePurple: I wish we outlived the need for that in C++/C. Unfortunately, "if(age = 20)" errors still exist. If C++ syntax outlaws that bug, we won't have to bother anymore. Alternative is to mask all c++ conditions in inline functions so it says "if( isTargetAge(age) )" instead.

    ReplyDelete
  5. There is a longer discussion on stack overflow:

    http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement

    Thanks to Tracy Harms for pointing it out.

    ReplyDelete
  6. This might be a bunny trail, but C/C++ always allowed multiple exit points from a function. It was Pascal and the languages that derived from that tradition that explicitly forbade multiple return statements. The complier enforced this rule. Those of us who started out in that environment had to re-evaluate the value of having a single return point when we migrated to C++, or at least I did.

    In the end I decided that having a quick return at the start of a function to catch and handle error conditions, etc. actually makes the code much more readable.

    ReplyDelete