Flatten conditional as a refactoring

Question

Consider:

if (something) {
    // Code...
}

With CodeRush installed it recommended doing:

if (!something) {
    return;
}
// Code...

Could someone explain how this is better? Surely there is no benefit what so ever.

Answer 1

One less level of nesting.

Answer 2

I don't think CodeRush is recommending it --- rather just offering it as an option.

Answer 3

In some cases, it's cleaner to validate all of your inputs at the beginning of a method and just bail out if anything is not correct. You can have a series of single-level if checks that check successively more and more specific things until you're confident that your inputs are good. The rest of the method will then be much easier to write, and will tend to have fewer nested conditionals.

Answer 4

IMO, it depends on if something or !something is the exceptional case. If there is a significant amount of code if something happens, then using the !something conditional makes more sense for legibility and potential nesting reduction.

Answer 5

It can be used to make the code more readable (by way of less nesting). See here for a good example, and here for a good discussion of the merits.

That sort of pattern is commonly used to replace:

void SomeMethod()
{
    if (condition_1)
    {
        if (condition_2)
        {
            if (condition_3)
            {
                // code
            }
        }
    }
}

With:

void SomeMethod()
{
    if (!condition_1) { return; }
    if (!condition_2) { return; }
    if (!condition_3) { return; }

    // code
}

Which is much easier on the eyes.

Answer 6

Isolated, as you've presented it - no benefit. But mark4o is right on: it's less nesting, which becomes very clear if you look at even, say a 4-level nesting:

public void foo() {
    if (a)
        if (b)
            if (c)
                if (d)
                    doSomething();
}

versus

public void foo() {
    if (!a)
     return;
    if (!b)
     return;
    if (!c)
     return;
    if (!d)
     return;
    doSomething();
}

early returns like this improve readability.

Answer 7

Well, look at it this way (I'll use php as an example):

You fill a form and go to this page: validate.php

example 1:

<?php

if (valid_data($_POST['username'])) {

    if (valid_data($_POST['password'])) {

     login();

    } else {
     die();
    }

} else {
    die();
}

?>

vs

<?php

if (!valid_data($_POST['username'])) {
    die();
}

if (!valid_data($_POST['password'])) {
    die(); 
}

login();

?>

Which one is better and easier to maintain? Remember this is just validating two things. Imagine this for a register page or something else.

Answer 8

This is a conventional refactoring meant for maintainability. See:

http://www.refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html

With one condition, it's not a big improvement. But it follows the "fail fast" principle, and you really start to notice the benefit when you have lots of conditions. If you grew up on "structured programming", which typically recommends functions have single exit points, it may seem unnatural, but if you've ever tried to debug code that has three levels or more of nested conditionals, you'll start to appreciate it.