“No-else-after-return” considered harmful

http://blog.mozilla.com/nnethercote/2009/08/31/no-else-after-return-considered-harmful/

by kinetik • 17 years ago

16 32 17 years ago

blog.mozilla.com

Profoundly unconvinced.

thristian• 17 years ago

On the one hand, this reads like "I prefer functional languages, and want to twist procedural languages into a form I'm more familiar with". On the other hand, I do find his re-written example more readable, and I'm not sure if that's due to my previous exposure to functional programming, some innate readability bestowed by the functional programming style, or just the fact that this particular code was rewritten by someone who wanted to make it a publishable example (as opposed to just getting it to work).

nostrademons• 17 years ago

C++ is not Ocaml, and Mozilla is not random-opensource-project. When you're contributing to an established project, you follow the conventions there even if they aren't what you'd consider ideal.

Brendan Eich even said as much in the comments of the bug: he'd rather be programming in a pure, expression-based manner too, but C++ is not Ocaml. And for a lot of C++ code, no-else-after-return works reasonably well.

psyklic• 17 years ago

A minor quibble. He claims that "no-elses" are prone to side-effects and "error-prone junk". However, "no-elses" are in my opinion as flagrant a crime as "no-braces" following if statements (which appears to be his style). The author says that he prefers the ternary operator (a > b ? a : b) to "no-elses"; however, its use is definitely more error-prone!

makecheck3• 17 years ago

I always go a step further, and use at most one "return" in a function.

I find short-cut "return" statements frustrating for many reasons. For example, there's a risk that proper cleanup will not always happen, it is harder to see state and program flow when debugging, and it's no longer simple to make every-case changes by adding things to the end of the function. They're really just another type of "goto".

The only "advantages" I can think of are saving a few keystrokes, and having slightly better performance by not using a variable for the return value. In my experience, neither of those things has ever mattered.

In other words, I'd do this:

  <type> result = defaultValue;
  if (condition)
  {
    result = val1;
  }
  else
  {
    result = val2;
  }
  return result;

With this approach, there is never anything after the "return", "else" or otherwise.

tptacek• 17 years ago

I write this way, religiously, and literally every teammate I've worked with has been frustrated by it. I agree with all your arguments (and I'll add another one, which is that it makes the code itself easier to instrument --- which is why I started doing it), but I've been convinced not to try to convince others of it.

Legion• 17 years ago

I tend to do this too, but I'm not convinced yet that I'm doing the right thing. I'd love to hear other opinions on this style.

cperciva• 17 years ago

I always go a step further, and use at most one "return" in a function.

I opt for one return per exit type. For example:

* If I have a success/failure function, I will have "return (0);" and "return (1);" statements.

* If I have a pointer-or-fail function (e.g., an object constructor), I will have "return (r);" and "return (NULL);" statements.

* If I have a true/false/fail function (e.g., check if a file exists; return 1 if it exists, return 0 if it doesn't exist, and return -1 if something failed such that we couldn't figure out if the file exists), I will have "return (0);", "return (1);", and "return (-1);" statements.

In trivial functions, this tends to make my code somewhat more verbose than necessary; but I've found that sticking to this coding style dramatically reduces the odds of me making mistakes when I come back to code later and modify it, since different exit types usually require different cleanup code.

mullr• 17 years ago

Your cleanup argument holds true for C and its ilk, but not for anything else. In C you DO have to do manual cleanup, but in any language with exceptions you can (and should) do that in a finally block. Or in C++, via one of the many scope-aware smart pointer classes. In higher level languages, it is idiomatically correct to have many exit points from a function.

As for being harder to debug... it depends. Again, if you MUST do manual resource deallocation, then I can see your case. Having duplicated cleanup code inside the control flow gets old fast. But if you've got exceptions, then such a rule leads very quickly to unnecessarily deep, convoluted control structures. I've flattened many such structures by a) factoring chunks out to separate functions and b) returning early when appropriate.

It is very common for a function/method to either throw an exception or return a default value based on some parameter. In these cases, it's pound-you-in-the-face clear to have a list of "if(check) return whatever;" clauses at the top. If you do this before resource allocation, you don't even need to worry about cleanup. This feels like a much "purer" way to write a function to me, almost like applying pattern matching before falling back to imperative mode.

makecheck3• 17 years ago

That's true, and I do use that approach ("RAII") in languages like C++. It definitely makes cleanup less of a problem. It also means that setup should be done no earlier than necessary, declaring variables very close to their first use (otherwise, a setup/cleanup cost may be paid for no reason).

Exceptions act like early returns themselves, but I will use them for truly exceptional cases (only). In other words, if a function can return 3 values, I consider those the "normal" cases that should weave through to the end of the function; if the function runs out of memory, I think that's an exceptional case that can return immediately.

antonovka2• 17 years ago

Yuueurrgh.

Without early return, you wind up with multiple-nested if statements that make the code absolutely unreadable. Early return is almost always used as the error path -- if you need to perform cleanup, then instead of returning, use goto to jump to a single end-of-function cleanup handler.

There's nothing wrong with using goto for this purpose, and it means I can actually read -- at a glance -- code that requires more than a few preconditions at the start of the function.

makecheck3• 17 years ago

Functions that have become too complicated should look ugly, so that they're likely to be split into simpler parts (for true readability).

When making that split, it is also helpful to have relevant cleanup code nearby: odds are everything is already in the inner block, instead of having to remove extra code at the end of the function.

anamax2• 17 years ago

> if you need to perform cleanup, then instead of returning, use goto to jump to a single end-of-function cleanup handler.

That assumes that all of the cleanup is the same and that all of the relevant information that is available at the subroutine's top level.

naz• 17 years ago

Just use whatever seems clearer

akkartik• 17 years ago

Yep. Use else when the two branches do symmetrical amounts of work. Don't use else if one is an early return.

If you find this discussion interesting you're probably stuck with codebases with overly long functions. My commiserations.

apgwoz• 17 years ago

What's his argument about purity here? His examples rely on global state which is inherently impure. Rewriting them to make it more expression like does nothing to change that.

redcap• 17 years ago

I think the main problem is the lack of braces means you have to rely on white space to figure out what the hell is going on.

That's the main reason why I religiously put open and close braces on even the smallest if/else blocks as it dramatically improves the readability of the code.

If the mozilla code style specified that you have to use braces no matter what then they wouldn't be having this problem.

Of course, I'm probably arguing with hackers who prefer to use constructs like 'return (a > b ? a : b)' rather than write readable code.

This is coming from someone who likes to put spaces after and before parentheses so it makes it easier to figure out what's going on.

e.g. if ( ( y > x ) || ( x < 10 ) ) - this makes things easier to read (for me) when you start to get a hell of a lot of nesting going.

philwelch4• 17 years ago

The ternary operator is perfectly readable if you spend even the slightest amount of time getting used to it.

I like spacing my parens too, but what you wrote isn't as readable to me as:

  ( (y > x) || (x < 10) )

Spacing inside your terminal pair of parens only makes your atomic expressions harder to read.

I also like multiline expressions:

  if (  (x == 10) ||
        (y == 10) ||
        (x > y) )
  {
    .....
  }

anamax2• 17 years ago

Why do you put the operators for multiline expressions at the end of the preceeding line instead of at the beginning of the next line?

With statements, the first token on the line tells you what it's doing and the indentation tells you about its relationship to statement on the previous line. Why not do something similar for multi-line expressions?

xsmasher• 17 years ago

I'm with Mozilla.

If I have a function with multiple conditions that cause me to return error / false / nil, I'd rather not have to nest them all in a series of if/then/else statements nor combine them into a single more-complicated if condition. I'd rather write them each as separate if-and-returns, followed by the real meat of the function.

function updatePlayers{

  if (charList is empty)
     return false;

  if (play is paused)
     return false;

  if (char is frozen)
     return false;

  //do stuff here
  return true;

}

I think this better spells out the "error" conditions.

philwelch4• 17 years ago

You'd be right, of course, but the original article concedes this point. He's talking about situations like

  if (a < b)
    return a
  return b

vs.

  if (a < b)
    return a
  else
    return b

  return ( (a < b) ? a : b)

neilc• 17 years ago

What is the distinction between those two situations? (i.e. those in which the article "concedes the point"?)

natrius• 17 years ago

Guard clauses are a particular case of if statements. I agree with omitting elses when using guard clauses. A blanket rule that eliminates elses after returns doesn't make any sense.

njharman• 17 years ago

Blanket rules don't make sense.

wlievens• 17 years ago

Error is the wrong term, because you don't return after an error condition, you raise an exception. Perhaps stop condition is more appropriate?

joecomotion• 17 years ago

I prefer to return as early as possible and I dislike unnecessary else clauses. And unnecessary braces. I especially hate really long functions where there's only one return but a bunch of baggage and nested conditionals along the way to pull it off. Like mullr said, only handy if your language doesn't support guards.

Most of the people who code this way that I've met don't do it b/c they're trying to be 'pure.' Most of them say it was coding standard some manager mentioned somewhere else long ago and now they just do it w/o thinking.

wglb• 17 years ago

I disagree. The rule works for me.

I now find myself using 'cond' almost exclusively over 'if'.

And there is a little misunderstanding propagated: "Even in a language like C/C++ you can do something approaching functional programming by avoiding statements". I don't think so.

ori_b• 17 years ago

And here we have a classical bikeshedding article.

This sort of thing really doesn't matter much one way or another. Do what the rest of the codebase is doing, and move on with your life. If you're writing your own code, do what you feel like doing.

“No-else-after-return” considered harmful

Related Stories