Why is "else" rarely used after "if x then return"?

Question

This method:

boolean containsSmiley(String s) {
    if (s == null) {
        return false;
    }
    else {
        return s.contains(":)");
    }
}

can equivalently be written:

boolean containsSmiley(String s) {
    if (s == null) {
        return false;
    }

    return s.contains(":)");
}

In my experience, the second form is seen more often, especially in more complex methods (where there may be several such exit points), and the same is true for "throw" as well as "return". Yet the first form arguably makes the conditional structure of the code more explicit. Are there any reasons to prefer one over the other?

(Related: Should a function have only one return statement?)

Solution 1

In my experience, it depends on the code. If I'm 'guarding' against something, I'll do:

if (inputVar.isBad()) {
    return;
}

doThings();

The point is clear: If that statement is false, I don't want the function to continue.

On the other hand, there are some functions with multiple options, and in that case I would write it like this:

if (inputVar == thingOne) {
    doFirstThing();
} else if (inputVar == secondThing) {
    doSecondThing();
} else {
    doThirdThing();
}

Even though it could be written as:

if (inputVar == thingOne) {
    doFirstThing();
    return;
}
if (inputVar == thingTwo) {
    doSecondThing();
    return;
}
doThingThree();
return;

It really comes down to which way most clearly shows what the code is doing (not necessarily which bit of code is shortest or has the least indentation).

Solution 2

The else in that case would be redundant, as well as create unnecessary extra indentation for the main code of the function.

Solution 3

This is a pattern called Guard Clause. The idea is do all the checking upfront to reduce nested conditions to increase readability.

From the link:

double getPayAmount() {
    double result;
    if (_isDead) {
        result = deadAmount();
    } else {
        if (_isSeparated) {
            result = separatedAmount();
        } else {
            if (_isRetired) {
                result = retiredAmount();
            } else {
                result = normalPayAmount();
            }
        }
    }

    return result;
}

Using the Guard Clause, you'll get to see this result:

double getPayAmount() {
    if (_isDead) return deadAmount();
    if (_isSeparated) return separatedAmount();
    if (_isRetired) return retiredAmount();

    return normalPayAmount();
};

Solution 4

You will see this all over:

if (condition) {
    return var;
}
// by nature, when execution reaches this point, condition can only be false,
// therefore, the else is unnecessary
return other_var;

Most of the time the addition of an else clause is not only unnecessary in this case, but a lot of times, it gets optimized away by the compiler.

Think of how the computer thinks of this code (in terms of machine code, simplified into pseudocode here for demonstration purposes):

0x00: test [condition]
0x01: if result of test was not true, goto [0x04]
0x02: push [var] onto stack
0x03: goto [0x05]
0x04: push [other_var] onto stack
0x05: return from subroutine

The code (again, this is a pseudocode and not assembly) would act the exact same way for an if/then/else conditional.

It is, by many people, considered bad and/or confusing practice to have multiple possible exit points for a function, as a programmer must think of every possible path through his code. Another practice is the following:

return (condition) ? var : other_var;

This simplifies the code, and does not create any new exit points.

Solution 5

I prefer writing it like this:

boolean containsSmiley(String s) {
    return s != null && s.contains(":)");
}

View more solutions

Comments

[Stephen C]

I note your smiley ... but comments are not a substitute for readable code. Comments are for when you can't make the code readable/understandable any other way.

[zneak]

I'm already surprised certain IDEs decide this is warning, but an error? Can we have names?

[G__]

+1 Eclipse does this. It's debatable whether its "correct" but because the IDE flags it as a warning by default, many will do this.

[Juha Syrjälä]

You can configure Eclipse to give error or warning about this. I don't remember what is the default.

[Drew Hall]

Who said anything about multiplying? :)

[LaSul]

PMD, on the other hand, warns about having multiple return statements by default.

[Anax]

Still, wouldn't switch be more appropriate in this case?

[Igor Zevaka]

You can't have function calls as switch labels. They have to be either literals or const s.

[atamanroman]

resharper warns about this, too.

[Anax]

Silly me, just noticed that :S

[Tim Bender]

+1 since this is a predicate and most predicates are one-liners... but it doesn't really answer the question

[Todd Owen]

Well, I'm sorry for positing the existence of the "else" clause! But Santa Claus is real, right? :)

[Todd Owen]

That's something I hadn't thought of, but wouldn't this warning to the reader (that there are multiple exit points in the function) be a good thing?

[Andrei Fierbinteanu]

Exactly, anything that is after the if clause is basically in an else statement anyway, since the only way to get there is to bypass the if by having the condition false, since if the condition is true, the method returns, and nothing else after the if is executed.

[Callum Rogers]

This example is exactly why we don't necessarily need a single exit point to everything.

[Todd Owen]

My take-away from all of this is: return statements near the start or the end of a function are probably fine, but return statements half-way through a complex function are a code smell (i.e. consider refactoring to reduce exit points).

[bl4ckb0l7]

From a performance point of view, isn't it a good idea to try to put the most regular case in front while using the Guard Clause to avoid unnecessary condition calls?

[Eugene Yokota]

@chpwn, patterns are nothing but vocabulary that's shared among some community for a practice that already exist in the field. I used to call this one "get out early," but it's nice to have some word that can convey the concept, so someone who knows it gets in a second.

[Eugene Yokota]

@bitschnau, from the performance point of view, it's a good idea to run the profiler and optimize only the code that matters.

[ldog]

Haha, well maybe I should have made things clearer. In this case you should interpret Occam's Razor as the principle that says 'when two solutions are presented to your problem, both equally valid, choose the simpler of the two.' In this case the option without the extra text is simpler.

[Brendan Long]

@Todd Owen: In general, I think that's a great way to put it. I think there are exceptions, but every style rule has exceptions.

[Grant Paul]

@eed3si9n: I'm actually fine with naming things, but I don't like the word "pattern"...it makes my code sound like it's the same as all other code ;P.

[Hamish Grubijan]

The later example sounds awfully close to a switch. Also, in Python I would just have a dictionary of values to lambdas, and do this: if not inputVar in input2Func: raise(...) # Implicit else :) x = input2Func[inputVar](). Other languages can allow something like this too ... the only trick is that signatures must match.

[user]

It only sounds close to a switch when you are doing a simple comparison on a single variable. At least Java won't allow you to switch on multiple variables or various complex conditions; then you need a set of if statements.

[Trojan]

Just speculation here, but your code "from the link" is a poor example. That code ought to be if(_isDead)...else if(_isSeparated)...else if (etc.), reducing all the indentation. Nesting all those if/elses is agreeably horrible, but somebody really examining their code well enough to be asking this question ought to have the sense to combine else{if/else} into else ifs.

[AlQuemist]

This does not answer the question and merely mentions some personal preference. Code clarity is also lost in this example.