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?

I've often written this sort of function in both formats, and I was wondering if one format is preferred over another, and why.

public void SomeFunction(bool someCondition)
{
    if (someCondition)
    {
        // Do Something
    }
}

or

public void SomeFunction(bool someCondition)
{
    if (!someCondition)
        return;

    // Do Something
}

I usually code with the first one since that is the way my brain works while coding, although I think I prefer the 2nd one since it takes care of any error handling right away and I find it easier to read

share|improve this question

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

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.

41  
I'm awfully tempted to re-tag this with "holy-war". –  Larry Coleman Nov 11 '10 at 19:22
19  
The real problem here is the bracket placement. ;) –  M2tM Nov 11 '10 at 22:42
7  
@M2tM: Why? It's perfect! ;-P –  peterchen Nov 11 '10 at 23:13
2  
I'm a bit late for this discussion so I won't put this in an answer; I also thought about this two years ago: lecterror.com/articles/view/code-formatting-and-readability I find the second easier to read, modify, maintain and debug. But maybe that's just me :) –  dr Hannibal Lecter Nov 12 '10 at 12:39
5  
Are they tabs or spaces... –  Jon Hopkins Feb 18 '11 at 13:33

19 Answers 19

up vote 128 down vote accepted

I prefer the second style. Get invalid cases out of the way first, either simply exiting or raising exceptions as appropriate, put a blank line in there, then add the "real" body of the method. I find it easier to read.

share|improve this answer
8  
+1 easier to read, imo. –  qes Nov 11 '10 at 19:36
36  
Exiting early allows you to pop stuff off your limited mental stack. :) –  Joren Nov 12 '10 at 7:59
11  
I've previously heard this referred to as "the Bouncer Pattern" - get rid of bad cases before they get in the door. –  RevBingo Nov 12 '10 at 21:02
4  
I'd even go as far and say this definitely is the ONLY right thing to do. –  Oliver Weiler Dec 6 '10 at 0:34
8  
Plus you don't keep adding indents if you get rid of the border cases at the beginning. –  doppelgreener Feb 18 '11 at 13:31

Definitely the latter. The former doesn't look bad right now, but when you get more complex code, I can't imagine anyone would think this:

public int SomeFunction(bool cond1, string name, int value, AuthInfo perms)
{
    int retval=SUCCESS;
    if (someCondition)
    {
        if(name != null && name != "")
        {
            if(value != 0)
            {
                if(perms.allow(name)
                {
                    // Do Something
                }
                else
                {
                    reval=PERM_DENY;
                }
            }
            else
            {
                retval=BAD_VALUE;
            }
        }
        else
        {
            retval=BAD_NAME;
        }
    }
    else
    {
        retval=BAD_COND;
    }
}
    }
}

is more readable than

public int SomeFunction(bool cond1, string name, int value, AuthInfo perms)
{
    int retval=SUCCESS;
    if (!someCondition)
    {
        return BAD_COND;
    }
    if(name == null || name == "")
    {
        return BAD_NAME;
    }
    if(value != 0)
    {
        return BAD_VALUE;
    }
    if( !perms.allow(name))
    {
        return PERM_DENY;
    }
    // do something
}

I fully admit I never understood the advantage of single exit points.

share|improve this answer
5  
The advantage of single a exit point is that... there's a single exit point! With your example, there's several points which could return. With a more complex function, that could turn into a hunt-the-exit-point when the format of the return value changes. Of course, there's times when forcing a single exit point doesn't make sense. –  JohnL Nov 11 '10 at 21:17
24  
@JohnL big functions are the problem, not multiple exit points. Unless you're working in a context where an additional function call will slow down your code immensely, of course... –  Yar Nov 11 '10 at 21:23
2  
@Yar: True, but the point stands. I'm not going to try and convert anyone, and I was just pointing out the advantage of going for as few exit points as possible (and Jason's example was kind of straw man anyway). Just next time you're pulling your hair out trying to figure out why SomeFunction sometimes returns odd values, maybe you'll wish you could add a logging call just before the return. Much easier to debug if there's only one of the buggers! :) –  JohnL Nov 11 '10 at 23:11
21  
@Jason Viers As if a single return value; helps!?! Then one has to hunt half a dozen value = ..., with the disadvantage that you are never sure that value will not be changed between this assignment and the final return. At least an immediate return is clear that nothing will change the result anymore. –  Sjoerd Feb 18 '11 at 17:26
2  
@Sjoed: I second Sjoerd here. If you want to log the result, you can log at the caller site. If you want to know the reason, you have to log at each exit point/assignment so both are identical in this aspect. –  Matthieu M. Feb 18 '11 at 18:46

It depends - In general I am not going to go out of my way to try and move a bunch of code around to break out of the function early - the compiler will generally take care of that for me. That said though, if there are some basic parameters at the top that I need and can't continue otherwise, I will breakout early. Likewise, if a condition generates a giant if block in function I will have it breakout early as a result of that as well.

That said though, if a function requires some data when it is called, I'm usually going to be throwing an exception (see example) as opposed to just returning.

public int myFunction(string parameterOne, string parameterTwo) {
  // Can't work without a value
  if (string.IsNullOrEmpty(parameterOne)) {
    throw new ArgumentNullException("parameterOne");
  } 
  if (string.IsNullOrEmpty(parameterTwo)) {
    throw new ArgumentNullException("parameterTwo");
  }

  // ...      
  // Do some work
  // ...

  return value;
}
share|improve this answer
6  
And if the end result is maintainable, who cares which style was selected? –  Jeff Siver Nov 11 '10 at 19:32
3  
@Jeff Siver - Thus why this tends to be a "holy-war" style question, at the end of the day it comes down to personal preference and whatever an internal style guide says. –  rjzii Nov 11 '10 at 19:33

I prefer the early return.

If you have one entry point and one exit point then you always have to track the entire code in your head all the way down to the exit point (you never know if some other piece of code bellow does something else to the result, so you have to track it up until the exist). You do that no mater which branch determines the final result. This is hard to follow.

With one entry and multiple exists, you return when you have your result and don't bother tracking it all the way down to see that nobody does anything else to it (because there won't be anything else since you returned). It's like having the method body split into more steps, which each step with the possibility to return the result or let the next step try its luck.

share|improve this answer

Personally, I prefer to do pass/fail condition checks at the beginning. That allows me to group most of the most common failures at the top of the function with the rest of the logic to follow.

share|improve this answer

It depends.

Early return if there is some obvious dead end condition to check for right away that would make running the rest of the function pointless.*

Set Retval + single return if the function is more complex and could have multiple exit points otherwise (readability issue).

*This can often indicate a design problem. If you find that a lot of your methods need to check some external/paramater state or such before running the rest of the code, that's probably something that should be handled by the caller.

share|improve this answer
5  
When I write code that may be shared, my mantra is "Assume Nothing. Trust No One." You should always validate your inputs and any external state you depend on. Better to throw an exception than possibly corrupt something because somebody gave you bad data. –  TMN Nov 11 '10 at 20:59
2  
Key point here is that in OO you throw an exception, not return. Multiple returns can be bad, multiple exception throws aren't necessarily a code smell. –  Michael K Nov 12 '10 at 13:59

I use both.

If DoSomething is 3-5 lines of code then the code just look beautiful using the first formatting method.

But if it has many more lines than that, then I prefer the second format. I don't like when the opening and closing brackets are not on the same screen.

share|improve this answer

Early returns for-the-win. They can seem ugly, but much less ugly than big if wrappers, especially if there's multiple conditions to check.

share|improve this answer

I use early returns almost exclusively these days, to an extreme. I write this

self = [super init];

if (self != nil)
{
    // your code here
}

return self;

as

self = [super init];
if (!self)
    return;

// your code here

return self;

but it really doesn't matter. If you have more than one or two levels of nesting in your functions, they need to be busted up.

share|improve this answer

Use an If

In Don Knuth's book about GOTO's I read him give a reason for always having the most likely condition come first in an if statement. Under the assumption that this is still a reasonable idea (and not one out of pure consideration for the speed of the era). I'd say early returns aren't good programming practice, especially considering the fact that they're more often than not used for error handling, unless your code is more likely to fail than not fail :-)

If you follow the above advice, you'd need to put that return at the bottom of the function, and then you might as well not even call it a return there, just set the error code and return it two lines hence. Thereby achieving the 1 entry 1 exit ideal.

Delphi Specific...

I'm of the mind that this is a good programming practice for Delphi programmers, although I don't have any proof. Pre-D2009, we don't have an atomic way to return a value, we have exit; and result := foo; or we could just throw exceptions.

If you had to substitute

if (true) {
 return foo;
} 

for

if true then 
begin
  result := foo; 
  exit; 
end;

you might just get sick of seeing that at the top of every one of your functions and prefer

if false then 
begin
  result := bar;

   ... 
end
else
   result := foo;

and just avoid exit altogether.

share|improve this answer
1  
With newer Delphis, it could be abbreviated to if true then Exit(foo); I use often the technique to first initialize result to nil or FALSE respectively, then checking all the error conditions and just Exit; if one is met. The success-case result is then (typically) set somewehere at the end of the method. –  JensG May 21 at 8:13

I agree with the following statement:

I'm personally a fan of guard clauses (the second example) as it reduces the indenting of the function. Some people don't like them because it results in multiple return points from the function, but I think it's clearer with them.

Taken from this question in stackoverflow.

share|improve this answer

In C programming where you have to manually clean-up there is a lot to be said for one-point return. Even if there is no need right now to clean something up, someone might edit your function, allocate something and need to clean it up before return. If that happens it will be a nightmare job looking through all the return statements.

In C++ programming you have destructors and even now scope-exit guards. All these need to be here to ensure the code is exception-safe in the first place, so code is well guarded against early exit and therefore doing so has no logical downside and is purely a style issue.

I am not knowledgeable enough about Java, whether "finally" block code will get called and whether finalizers can handle the situation of needing to ensure something happens.

C# I certainly can't answer on.

D-language gives you proper built-in scope-exit guards and therefore is well-prepared for early exit and therefore should not present an issue other than style.

Functions should of course not be so long in the first place, and if you have a huge switch statement your code is probably also badly factored.

share|improve this answer

I would prefer to write:

if(someCondition)
{
    SomeFunction();
}
share|improve this answer
2  
eww. Is that pre-validation? Or is it in a validation-dedicated mehtod DoSomeFunctionIfSomeCondition? –  STW Nov 11 '10 at 19:27
2  
You're violating encapsulation by making the function's implementation (its dependency logic) external. –  TMN Nov 11 '10 at 20:45
1  
If this is run in a public method and SomeFunction() is a private method in the same class it could be ok. But if anyone else is calling SomeFunction() you would have to duplicate the checks there. I find it better to make each method take care of what it needs to do its work, no one else should have to know that. –  Per Wiklander Nov 11 '10 at 23:34
2  
This is definitely the style that Robert C. Martin proposes in "Clean Code". A function should only do one thing. However in "Implementation Patterns", Kent Beck suggests, of the two options proposed in the OP, the second is better. –  Scott Whitlock Nov 13 '10 at 1:20

A classic reason for single-entry-single-exit is that otherwise the formal semantics become unspeakably ugly otherwise (same reason GOTO was considered harmful).

Put another way, it's easier to reason about when your software will exit the routine if you have only 1 return. Which is also an argument against exceptions.

Typically I minimize the early-return approach.

share|improve this answer

As others say, it depends. For little functions that return values, I may code early returns. But for sizeable functions, I like to always have a place in the code where I know I can put something that will get executed before it returns.

share|improve this answer

Like you, I usually write the first one, but prefer the last one. If i have a lot of nested checks i usually refactor to the second method.

I don't like how the error handling is moved away from the check.

if not error A
  if not error B
    if not error C
      // do something
    else handle error C
  else handle error B
else handle error A

I prefer this:

if error A
  handle error A; return
if error B
  handle error B; return
if error C
  handle error C; return

// do something
share|improve this answer

The conditions on the top are called "preconditions". By putting if(!precond) return;, you are visually listing all preconditions.

Using the large "if-else" block may increase indent overhead (I forgot the quote about 3-level indents) and generate more bytecode.

Also, what language type is this question talking about? In Java and other strongly-typed languages, you cannot do early returns (but you can throw exceptions). In JavaScript, early returns may generate warnings.

share|improve this answer

I prefer to keep if statements small.

So, choosing between:

if condition:
   line1
   line2
    ...
   line-n

and

if not condition: return

line1
line2
 ...
line-n

I'd choose what you described as "early return".

Mind you, I don't care about early returns or whatever, I just really like to simplify the code, shorten the bodies of if statements, etc.

Nested if's and for's and while's are horrible, avoid them at all costs.

share|improve this answer

I practice fail-fast at the function level. It keeps code consistent and clean (to me and those I've worked with). Because of that I always return early.

For some oft-checked conditions, you can implement aspects for those checks if you use AOP.

share|improve this answer

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