Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.
share|improve this question
4  
Basically "it depends" and "be consistent" is the advice I'd offer. This question doesn't really have a single answer. –  ChrisF Aug 29 '11 at 23:12
    
re your point about svn: One of the cool things about git-svn is that you can do git diff --ignore-space-change to produce nicer diffs for this kind of thing. –  MatrixFrog Aug 30 '11 at 2:16
4  
"Con: Requires you to read the whole function to figure out the various run paths." -- I'd argue you still have to do this with the if-clauses, if slightly differently. –  zenzelezz Aug 30 '11 at 8:22
    
Either way is fine if you comment a lot. I prefer to use premature return. If you'd use more than one wrap-in if clauses, put comment lines above the closing braces to indicate which method it belongs to.. //successfulCondition() closing } //doIt() closing } –  Vigneshwaran Sep 19 '11 at 3:52
    
"Requires you to read the whole function" isn't as big of a deal if you write succinct methods. –  Jeff Bridgman Jun 11 '13 at 14:08
add comment

marked as duplicate by Mark Trapp Nov 10 '11 at 1:57

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

7 Answers

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 http://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.

share|improve this answer
1  
+1 I can handle returning at the beginning of the method/function or at the end; it is easy to overlook in the middle of some convoluted code (probably I) wrote a while back. –  Ken Henderson Aug 30 '11 at 0:57
1  
Guard clauses are good, returns in the middle aren't. If you truly need them (I think I've written one in the last year) then document it--the routine I mentioned has a comment at the bottom saying where the actual return is. –  Loren Pechtel Aug 30 '11 at 3:51
add comment

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.

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.

share|improve this answer
    
WHat about if(!userHasCream()) if(IHaveMilk()) pourMilkIntoMug(); else return - is this bad or good? I say bad. –  mattnz Aug 29 '11 at 23:58
    
@mattnz - I'd avoid performing a return in the middle if possible. However if you do need to return in the middle (I probably have only ever done that once in the past 4 years). It will be easier to find/read in a self-commenting code format than a 100+ line method with multiple returns. Besides, in your example you would check to see if you require milk in the validation portion of the function. If you are out of milk validation might throw an exception throw MilkNotFoundException("Sorry you're out of milk"); –  Justin Shield Aug 30 '11 at 5:29
add comment

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.

share|improve this answer
2  
-1. Code is written once and read thousands of times. If you work in 1.5million line code bases such as me, its not lazy to scan code quickly. –  mattnz Aug 29 '11 at 23:54
1  
the theory of "structured programming" allows forward GOTOs (see: pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pdf) Nesting is acceptable for few conditions, otherwise, it becomes extremely difficult to follow even during debugging. The real problem that is not mentioned in the question is what about the error messages that should be returned. Having 1 error raised at a single point is indeed better than raising the same error from different places. –  Emmad Kareem Aug 30 '11 at 0:18
5  
@mattnz: thats the point. If code is read many times it needs to be easy to read. In many cases use of single return actually makes the code harder to read. –  quickly_now Aug 30 '11 at 0:44
1  
@quickly_now a single return COULD be hard to read and then again multiple returns COULD be hard to read. So shouldn't it be more about whether the style makes it hard to read in the code? rather than the actual style? –  James Khoury Aug 30 '11 at 3:56
1  
@James Khoury: Yes. Exactly. This is why its a judgement call as to which way you should go. I've also pointed out that much of what I've done for the last 10 years has been clearer by using multiple returns. Thats much... not all. I would suggest when writing code where this is an issue to stop and pause and see if is getting ugly. If its ugly to write it will probably be uglier still to read and understand. So if its ugly, try to un-ugly it. Sometimes multiple returns do that, sometimes not. –  quickly_now Aug 30 '11 at 6:54
show 1 more comment

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;
}
share|improve this answer
    
It takes a while (plus the comment) to understand this abuse of a loop. -1 from me, because this might be clever, but is non-intuitive and could be solved better by using other, less confusing idioms (finally, RAII). –  sbi Nov 9 '11 at 14:27
    
Everything is non-intuitive until you see it three times. Then it becomes a pattern. The least confusing idiom is the one which allows you to see what is going on with a quick scan and without having to drill up and down through multiple nexted blocks. –  Joel Brown Nov 9 '11 at 17:59
add comment

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.

share|improve this answer
    
Performance is never an issue in this sort of scenario. i.e. nested ifs vs premature returns. And even if it is, it will be minuscule to the point of irrelevance. –  Zoran Pavlovic Jul 4 '12 at 10:50
add comment

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?".

share|improve this answer
1  
Just put the breakpoint where the call is. Or use tests to test your method. –  user unknown Aug 30 '11 at 2:34
    
You put the breakpoint where you THINK it should stop ... but then it doesn't and goes sailing on by ... And Yes you write the test to test the method, but at midnight, its crashed, your clients are waiting on the build to fix their issue and stop loosing $1000 per per minute ... put the break point on check the value, step backwards through the code > nail the problem (usually remove the second NOT in NOT NOT X) > Compile > Test > Deploy is the shortest and cheapest path. You can then do the formal stuff the next day when you refactor your fix with a clear head and put in a formal solution. –  Robin Vessey Aug 30 '11 at 2:58
    
This is especially true when you have 5 or 10 possible candidate methods and you want to check where you ware up to Run > pause > check > run > pause > Ah HA. –  Robin Vessey Aug 30 '11 at 2:59
    
Well - that's an argument from movie-plot. You clearly should test your program always, to return the correct result. If you're tired, late at night under high pressure, everything can go wrong. If you don't have a "only one return statement"-policy, you know that, it isn't a surprise. I could as well claim, that the problem is, that you have fixed your result in line 4 of the method, but because you're not allowed to return it early, you go for 20 additional lines (by the way - why is this method so long?), and there, in line 17 you change the result accidentally. –  user unknown Aug 30 '11 at 8:54
    
Its not a movie plot, we fix other peoples code for a living ... this is what we do and when we write our own code we try to minimise the surface area for problems and this is just one of the things on a very big list. EG running a 200 seat call centre in real time, your app dies there are 200 people not doing any work ... Automatica layout of newspapers ... if a your daily newspaper didn't come out tomorrow how much do you think it would cost? And yeah as you say short methods are also on the list, much higher than this rule. –  Robin Vessey Aug 30 '11 at 9:22
show 3 more comments

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();

}

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.