Coding style: use early returns?

[Vyacheslav Ostapenko]

It is not written directly in chromium coding style.

What is preffered:

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

or

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

// Do Something
}

Thanks

[Brett Wilson]

Whatever you think is more clear for your case.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev

[Balazs Kelemen]

I think the situation which is more interesting here is when the condition is a check for some sort of abnormal case, for example a NULL check of a pointer, and the block consist of at least a couple of lines.

i.e.

  if (usually_not_null_ == NULL)
    return;
  // do stuff with usually_not_null_
  // more stuff...
  // a lot of stuff...


In Blink there is an explicit point in the style guide to use early return in this case.

Do you think that is also the norm in chromium, or is it rather just a matter of taste?

Thanks,

Balazs

[Scott Hess]

My experience is that most reviewers prefer early returns. Definitely
you should use early returns if the surrounding code is using early
returns.

Usually this is most strongly encouraged when the condition case(s)
are long. If the entire function is only three or four lines, it
matters less, but if the conditional case is a few dozen lines
(perhaps with further nested conditions), it is more valuable to strip
the layer of nesting.

Personally, I tend to prefer early returns with simple conditions over
convoluted and nested conditions. I find that it helps me see the
meat of the function more easily without getting distracted by edge
cases.

-scott

On Wed, Jun 24, 2015 at 10:38 AM, Vyacheslav Ostapenko
<ost...@gmail.com> wrote:

[Peter Kasting]

Scott's comments are close to what I'd say.  If an early return will shorten the code overall or reduce the nesting of long or deeply-nested blocks of code, it's probably preferable.  If it won't do either of those things, it probably doesn't matter.

I don't think Balazs' comment about the connection between early returns and the likelihood of taking a branch is something that is considered in most Chromium code, nor do I think it should be.  This is really subtle and easy to get wrong, and it's also easy for readers who expect this to misinterpret code.

PK

[Rachel Blum]

FWIW: If it _matters_ what likelihood a branch has, you should probably annotate that via the UNLIKELY macro so the next person will know why things are the way they are.

You probably should have some sort of evidence the probability matters, too :)

--

[Elliott Sprehn]

We prefer early return in Blink because it's easier to follow the code and it makes for nicer diffs when you add/remove conditions or re-order them. It also means you don't end up with functions where the entire body of the function is indented. I'd think that'd matter even more in chromium code where we have that silly 80 column limit. :)

[Sven Panne]

"Mr. Refactoring" himself proposes early returns, too: http://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html

IMHO it's very nice to have straight-line code which handles increasingly more general cases, with the specialized cases returning early. In other words, it's not so much about likelihood, but more about generality.

[Matt Menke]

I think it really depends on the situation.  If more code were added to the end of the method, how certain are you that you wouldn't want to do it in the early return case?

There's a big difference between:

if (error)

  return;

// Handle non-error case

and:

if (need_to_init_foo) {

  // Init foo.

} else {

  // Init something else instead, or do nothing?

}

// No code currently here, but if there were, would probably want to do it in both cases.

[Matt Giuca]

Totally second Matt's point. I think the key phrasing there is "make clear what the normal path of execution is".

I see a lot of Chromium code / reviewers that blindly follow the "early return" rule, as in "if you have two paths that never re-converge, do an early-return in one path and move the other path out of the if statement." This means we end up refactoring code like this:

if (cond1) {

  // Perfectly "normal" case.

} else if (cond2) {

  // Another perfectly "normal" case.

} else {

  // Another perfectly "normal" case.

}

Into this:

if (cond1) {

  // Perfectly "normal" case.

  return;

} else if (cond2) {

  // Another perfectly "normal" case.

  return;

}

// Another perfectly "normal" case.

This gives an undue significance to the final condition. If all three conditions are standard behaviour (not errors or special cases), then I think it's much easier to read and add new cases if all the cases are indented at the same level.

I'm a big fan of early returns, but they should be reserved for error cases and other special cases where you want to stop doing whatever algorithm you would otherwise do.

--

[Ryan Sleevi]

On Thu, Jun 25, 2015 at 6:58 PM, Matt Giuca <mgi...@chromium.org> wrote:

Totally second Matt's point. I think the key phrasing there is "make clear what the normal path of execution is".

I see a lot of Chromium code / reviewers that blindly follow the "early return" rule, as in "if you have two paths that never re-converge, do an early-return in one path and move the other path out of the if statement." This means we end up refactoring code like this:

if (cond1) {

  // Perfectly "normal" case.

} else if (cond2) {

  // Another perfectly "normal" case.

} else {

  // Another perfectly "normal" case.

}

Into this:

if (cond1) {

  // Perfectly "normal" case.

  return;

} else if (cond2) {

  // Another perfectly "normal" case.

  return;

}

// Another perfectly "normal" case.

This gives an undue significance to the final condition. If all three conditions are standard behaviour (not errors or special cases), then I think it's much easier to read and add new cases if all the cases are indented at the same level.

I'm a big fan of early returns, but they should be reserved for error cases and other special cases where you want to stop doing whatever algorithm you would otherwise do.

That second example wouldn't be consistent with the style guide - https://www.chromium.org/developers/coding-style#Code_Formatting - Don't use else after return

if (cond1) {

  // Perfectly "normal" case.

  return;

}

if (cond2) {

  // Another perfectly "normal" case.

  return;

}

// Another perfectly "normal" case.

That said, I agree with you that "if you want to stop doing whatever you're doing, use an early return", but that neither argues for nor against either pattern (if/else || if/return), since you can make reasonable and rational arguments for either pattern in that case, and it's a judgement call about trying to ensure the code is readable.

[Matt Giuca]

Sorry, for the second example I meant this:

if (cond1) {

  // Perfectly "normal" case.

  return;

}

if (cond2) {

  // Another perfectly "normal" case.

  return;

}

// Another perfectly "normal" case.

The point still stands: the third case looks unduly "more mainstream" than the first two, even though it just happens to be the last case (for now). I've had situations where I've wanted to add a fourth case and had to indent the third case, resulting in silly diffs.

[Peter Kasting]

On Thu, Jun 25, 2015 at 7:49 PM, Matt Giuca <mgi...@chromium.org> wrote:

The point still stands: the third case looks unduly "more mainstream" than the first two, even though it just happens to be the last case (for now).

I think this is another case of the reader bringing an expectation to the code.  When I read this, for example, I don't react that way at all, because I have never used early returns to mean "unusual condition"; I simply use them to reduce indenting, and I try to put the shorter conditional body as the early return.

I don't know what to do with that observation, though; I'm not going to start suddenly assuming early returns mean "exceptional case" any more than you're going to stop assuming they do, and the two of us will probably continue to both write and review code differently as a result.

I've had situations where I've wanted to add a fourth case and had to indent the third case, resulting in silly diffs.

I'm reluctant to ever make a formatting decision based on what future diffs might look like.

PK