Should a function use premature returns or wrap everything in if clauses?

Question

Possible Duplicate:
Where did the notion of “one return only” come from?

Which is better? I see pros and cons for both, so I can't really decide on which one to stick to.

Wrap in if clause

function doIt() {
   if (successfulCondition) {
      whenEverythingGoesWell();
   }
}

• Pro: Shows programmer's intention through indentation.
• Con: Indentation can get really deep if you need to short circuit many times. For example, doThirdThing() requires the success of doSecondThing(), which in turn requires the success of doFirstThing(). This happens a lot in web development where many web services are not reliable.

Premature return

function doIt() {
   if (!successfulCondition) {
      return;
   }
   whenEverythingGoesWell();
}

• Pro: Subversion checkins would be succinct. Sometimes, I see co-workers wrap super long functions in an if clause. The whole shebang gets checked in and makes reading Subversion diffs difficult.
• Con: Requires you to read the whole function to figure out the various run paths.

Answer 1

Martin Fowler talks about "Replace Nested Conditional with Guard Clauses" refactoring and generally advocates the clarity of quitting early for bad parameters over putting the whole method in an else clause (and see also https://stackoverflow.com/questions/4887212/shall-i-use-guard-clause-and-try-to-avoid-else-clause., which mentions that the traditional case for only exiting at the end was partly for resource clean up).

I think it's fairly easy and expected when I find a return at the beginning of a function, but I occasionally miss them buried in the middle, so I avoid putting returns in the middle unless it's really ugly to avoid them.

Answer 2

Premature return

Premature return is the way I recommend writing functions. This makes it clear at the top of the function that if you have invalid parameters that it will terminate early and not process further into the function. This also reduces cyclomatic complexity.

• Coding Horror: Flattening Arrow Code

Jeff Atwood (above) describes excellent methods to reduce cyclomatic complexity.

function doIt() {
   if (!successfulCondition) {
      return;
   }
   whenEverythingGoesWell();
}

Con: Requires you to read the whole function to figure out the various run paths.

This Con can and should very easily be rectified with minor refactoring.

Refactor your methods into small succinct functions which perform discrete operations. This will turn your whenEverythingGoesWell function into a list of private function calls which will read like pseudoCode.

Example

function MugOfCoffee doIt(int param1, string param2) {
   if (!successfulCondition(param1, param2) 
      return;

   boilKettle();
   getMilkFromFridge();

   getMugsFromCuppboard();

   if(userHasCream())
      pourMilkIntoMug();

   putCoffeeInMug();
   purHotWaterIntoMug();

   return new MugOfCoffee();
}

private bool successfulCondition(int param1, string param2)
{
   // Validation logic here
   return true;
}

At least each of the function names represents a step in a larger method and reads like it would in pseudocode. If the pourHotWaterIntoMug method changes it's loosely coupled from the doIt function and so will make it far far easier to read.

Answer 3

Oh dear, this has been the subject of university courses on program purity, and corporate coding standards, since about the time that Adam was a little tiny chap.

The major reason for a single point of return is that it means you don't have to hunt the source trying to find hidden exits (or so it is claimed).

What this actually means is you can be lazy when reading the source.

Now, the idea of being lazy when reading the source is the you can only concentrate on the bits that you think "matter". Whatever that is.

Trouble with all this is, the use of single returns can make the program / function flow and coding extremely convoluted. (Reminds me of code I wrote years ago where a function grew by 10 lines of VERY obscure and horrible code in order to avoid a "goto").

Sometimes, doing "naughty" things is right because it makes the result simpler and easier to understand. So if this means using the occasional goto, or using multiple returns, then you do so. But only when the alternative is worse. This can require some careful thought and might be a bit of a judgement call.

These days I tend to use multiple returns pretty much as a matter of course. The reason is that the "lazy" approach to reading code generally gets you nowhere. You DO need to read a function / method from the start to the end, so the multiple returns are pretty apparent - and doubly so if you put big comments in against each (which I do). So the lazy argument really does not hold water.

---

Nitpickers corner: Just because I mention gotos does not make me a cowboy or nutcase. I think I've managed to use "goto" twice in about the last 25 years. The point still stands, though: If its the best solution, use it.

Answer 4

Sometimes you have problems with either approach. You may be tempted to put in a bunch of guard clauses but at the same time you may have resources that you need to be sure to clean up.

Something I do when there are lots of possible things to do before getting to the meat of the method, especially when I want to have an orderly exit is to us a do/loop with a guaranteed break at the end. This is sort of a compromise premature exit. Instead of exiting out of the method, and instead of nesting many, many if blocks, you have one do/loop block which you can break out of if something bad happens.

It looks something like this...

public bool YourMethod()
{
    do    // For bailing, not for looping.
    {
        if (FirstBadThingHappens)
        {   // Do any logging or other follow up.
            break;
        }
        ...
        if (NthBadThingHappens)
        {   // And so forth...
            break;
        }
        // If we're still here then it must be OK to proceed...
        DoTheMeatOfTheMethod();

    } while(false)   // Always only one pass
    // Do whatever needs doing to wrap up cleanly...
    CleanUpResourcesOrWhatever();
    return bResult;
}

Answer 5

Good question, one that I've often asked myself. But I always answer that exiting is cleaner than the burden of conditional logic. Just make sure that your intent is clear - it is in your example.

Also: well thought-out and well placed assertions are sometimes clearer and more effective in such situations - just make sure that you appropriately handle the potential assertion failure in your caller.

function doIt() {

  Debug.Assert(successfulCondition,"condition not met");
  whenEverythingGoesWell();

}

Answer 6

Personally I have found the first way (which you have titled "Wrap in if clause") to be my preferred way. If I need to mitigate your con about deep indentation, I simply use a Boolean flag to keep track if doFirstThing() completed successfully. For me, it is all about code readability. I find it hard to track down multiple returns, as opposed to dealing with a little further indentation.

However, some situations might call for a greater level of performance, and therefor would benefit from a "Premature return". In those situations you would have another pro and con to weigh.

You decision should be based on: 1. What makes the code easiest to read?; 2. How important is optimization for performance? I cannot give you a single answer because other d other details about your usage are needed.

Answer 7

My vote is for NOT doing premeture returns.

Why? Primarily maintainablity. Code paths are a pain and debugging someone elses (or my own) mess 6 months later is much is eaiser if you can break point the one line at the bottom of the method saying "regardless of the path, this is what I'm returning ... is that what is expected?".