Is returning early from a function more elegant than an if statement?

Question

Myself and a colleague have a dispute about which of the following is more elegant. I won't say who's who, so it is impartial. Which is more elegant?

public function set hitZone(target:DisplayObject):void
{
    if(_hitZone != target)
    {
        _hitZone.removeEventListener(MouseEvent.ROLL_OVER, onBtOver);
        _hitZone.removeEventListener(MouseEvent.ROLL_OUT, onBtOut);
        _hitZone.removeEventListener(MouseEvent.MOUSE_DOWN, onBtDown);

        _hitZone = target;

        _hitZone.addEventListener(MouseEvent.ROLL_OVER, onBtOver, false, 0, true);
        _hitZone.addEventListener(MouseEvent.ROLL_OUT, onBtOut, false, 0, true);
        _hitZone.addEventListener(MouseEvent.MOUSE_DOWN, onBtDown, false, 0, true);
    }
}

...or...

public function set hitZone(target:DisplayObject):void
{
    if(_hitZone == target) return;

    _hitZone.removeEventListener(MouseEvent.ROLL_OVER, onBtOver);
    _hitZone.removeEventListener(MouseEvent.ROLL_OUT, onBtOut);
    _hitZone.removeEventListener(MouseEvent.MOUSE_DOWN, onBtDown);

    _hitZone = target;

    _hitZone.addEventListener(MouseEvent.ROLL_OVER, onBtOver, false, 0, true);
    _hitZone.addEventListener(MouseEvent.ROLL_OUT, onBtOut, false, 0, true);
    _hitZone.addEventListener(MouseEvent.MOUSE_DOWN, onBtDown, false, 0, true);
}

Answer 1

In most cases, returning early reduces the complexity and makes the code more readable.

It's also one of the techniques applied in Spartan programming:

Minimal use of Control

1. Minimizing the use of conditionals by using specialized constructs such ternarization, inheritance, and classes such as Class Defaults, Class Once and Class Separator
2. Simplifying conditionals with early return.
3. Minimizing the use of looping constructs, by using action applicator classes such as Class Separate and Class FileSystemVisitor.
4. Simplifying logic of iteration with early exits (via return, continue and break statements).

In your example, I would choose option 2, as it makes the code more readable. I use the same technique when checking function parameters.

Answer 2

This is one of those cases where it's ok to break the rules (i.e. best practices). In general you want to have as few return points in a function as possible. The practical reason for this is that it simplifies your reading of the code, since you can just always assume that each and every function will take its arguments, do its logic, and return its result. Putting in extra returns for various cases tends to complicate the logic and increase the amount of time necessary to read and fully grok the code. Once your code reaches the maintenance stage then multiple returns can have a huge impact on the productivity of new programmers as they try to decipher the logic (its especially bad when comments are sparse and the code unclear). The problem grows exponentially with respect to the length of the function.

So then why in this case does everyone prefer option 2? It's because you're are setting up a contract that the function enforces through validating incoming data, or other invariants that might need to be checked. The prettiest syntax for constructing the validation is the check each condition, returning immediately if the condition fails validity. That way you don't have to maintain some kind of isValid boolean through all of your checks.

To sum things up: we're really looking at how to write validation code and not general logic; option 2 is better for validation code.

Answer 3

As long as the early returns are organized as a block at the top of the function/method body, then I think they're much more readable than adding another layer of nesting.

I try to avoid early returns in the middle of the body. Sometimes they're the best way, but most of the time I think they complicate.

Also, as a general rule I try to minimize nesting control structures. Obviously you can take this one too far, so you have to use some discretion. Converting nested if's to a single switch/case is much clearer to me, even if the predicates repeat some sub-expressions (and assuming this isn't a performance critical loop in a language too dumb to do subexpression elimination). Particularly I dislike the combination of nested ifs in long function/method bodies, since if you jump into the middle of the code for some reason you end up scrolling up and down to mentally reconstruct the context of a given line.

Answer 4

In my experience, the issue with using early returns in a project is that if others on the project aren't used to them, they won't look for them. So early returns or not - if there are multiple programmers involved, make sure everyone's at least aware of their presence.

I personally write code to return as soon as it can, as delaying a return often introduces extra complexity eg trying to safely exit a bunch of nested loops and conditions.

So when I look at an unfamiliar function, the very first thing I do is look for all the returns. What really helps there is to set up your syntax colouring to give return a different colour from anything else. (I go for red.) That way, the returns become a useful tool for determining what the function does, rather than hidden stumbling blocks for the unwary.

Answer 5

As said before, early return is more readable, specially if the body of a function is long, you may find that deleting a } by mistake in a 3 page function (wich in itself is not very elegant) and trying to compile it can take several minutes of non-automatable debugging.

It also makes the code more declarative, because that's the way you would describe it to another human, so probably a developer is close enough to one to understand it.

If the complexity of the function increases later, and you have good tests, you can simply wrap each alternative in a new function, and call them in case branches, that way you mantain the declarative style.

Answer 6

Ah the guardian.

Imho, yes - the logic of it is clearer because the return is explicit and right next to the condition, and it can be nicely grouped with similar structures. This is even more applicable where "return" is replaced with "throw new Exception".

Answer 7

I prefer to avoid an immediate return at the beginning of a function, and whenever possible put the qualifying logic to prevent entry to the method prior to it being called. Of course, this varies depending on the purpose of the method.

However, I do not mind returning in the middle of the method, provided the method is short and readable. In the event that the method is large, in my opinion, it is already not very readable, so it will either be refactored into multiple functions with inline returns, or I will explicitly break from the control structure with a single return at the end.

Answer 8

I've had this debate with my own code over the years. I started life favoring one return and slowly have lapsed.

In this case, I prefer option 2 (one return) simply because we're only talking about 7 lines of code wrapped by an if() with no other complexity. It's far more readable and function-like. It flows top to bottom. You know you start at the top and end at the bottom.

That being said, as others have said, if there were more guards at the beginning or more complexity or if the function grows, then I would prefer option 1: return immediately at the beginning for a simple validation.

Answer 9

In this case (one test, no else clause) I like the test-and-return. It makes it clear that in that case, there's nothing to do, without having to read the rest of the function.

However, this is splitting the finest of hairs. I'm sure you must have bigger issues to worry about :)

Answer 10

option 2 is more readable, but the manageability of the code fails when a else may be required to be added.

So if you are sure, there is no else go for option 2, but if there could be scope for an else condition then i would prefer option 1

Answer 11

Option 1 is better, because you should have a minimal number of return points in procedure. There are exceptions like

  if (a) {
    return x;
  }
  return y;

because of the way a language works, but in general it's better to have as few exit points as it is feasible.

Answer 12

I am tempted to close it as exact duplicate, as I saw some similar threads already, including Invert “if” statement to reduce nesting which has good answers.

I will let it live for now... ^_^

To make that an answer, I am a believer that early return as guard clause is better than deeply nested ifs.

Answer 13

I have seen both types of codes and I prefer first one as it is looks easily readable and understandable for me but I have read many places that early exist is the better way to go.

Answer 14

There's at least one other alternative. Separate the details of the actual work from the decision about whether to perform the work. Something like the following:

public function setHitZone(target:DisplayObject):void
    {
        if(_hitZone != target)
            setHitZoneUnconditionally(target);

    }

public function setHitZoneUnconditionally(target:DisplayObject):void
    {
        _hitZone.removeEventListener(MouseEvent.ROLL_OVER, onBtOver);
        _hitZone.removeEventListener(MouseEvent.ROLL_OUT, onBtOut);
        _hitZone.removeEventListener(MouseEvent.MOUSE_DOWN, onBtDown);

        _hitZone = target;

        _hitZone.addEventListener(MouseEvent.ROLL_OVER, onBtOver, false, 0, true);
        _hitZone.addEventListener(MouseEvent.ROLL_OUT, onBtOut, false, 0, true);
        _hitZone.addEventListener(MouseEvent.MOUSE_DOWN, onBtDown, false, 0, true);

    }

Any of these three (your two plus the third above) are reasonable for cases as small as this. However, it would be A Bad Thing to have a function hundreds of lines long with multiple "bail-out points" sprinkled throughout.