Invert "if" statement to reduce nesting

Question

When I ran Resharper on my code, for example:

    if (some condition)
    {
        some code...

    }

Resharper gave me the above warning (Invert "if" statement to reduce nesting), and suggested the following correction:

   if (!some condition) return;
   some code...

I would like to understand why that's better. I always thought that using "return" in the middle of a method problematic, somewhat like "goto".

Update: I could only choose one correct answer, but almost all the answers were great (even if they contradicted each other....). Thanks everyone!

Answer 1

That is simply controversial. There is no "agreement among programmers" on the question of early return. It's always subjective, as far as I know.

It's possible to make a performance argument, since it's better to have conditions that are written so they are most often true; it can also be argued that it is clearer. It does, on the other hand, create nested tests.

I don't think you will get a conclusive answer to this question.

Answer 2

It's a matter of opinion.

My normal approach would be to avoid single line ifs, and returns in the middle of a method.

You wouldn't want lines like it suggests everywhere in your method but there is something to be said for checking a bunch of assumptions at the top of your method, and only doing your actual work if they all pass.

Answer 3

This is a bit of a religious argument, but I agree with Resharper that you should prefer less nesting. I believe that this outweighs the negatives of having multiple return paths from a function.

The key reason for having less nesting is to improve code readability and maintainability. Remember that many other developers will need to read your code in the future, and code with less indentation is generally much easier to read.

Preconditions are a great example of where it is okay to return early at the start of the function. Why should the readability of the rest of the function be affected by the presence of a precondition check?

As for the negatives about returning multiple times from a method - debuggers are pretty powerful now, and it's very easy to find out exactly where and when a particular function is returning.

Having multiple returns in a function is not going to affect the maintainance programmer's job.

Poor code readability will.

Answer 4

I think it depends on what you prefer, as mentioned, theres no general agreement afaik. To reduce annoyment, you may reduce this kind of warning to "Hint"

Answer 5

This is of course subjective, but I think it strongly improves on two points:

• It is now immediately obvious that your function has nothing left to do if condition holds.
• It keeps the nesting level down. Nesting hurts readability more than you'd think.

Answer 6

I really hate multiple return point from a method which aren't in separate blocks. Ive disabled that resharper notice.

It makes the method really hard to understand, you might be glancing over the method to figure out what it does, and miss the fact that because youve "reduced nesting" you think the code youre reading is in the main body of the method and so will run all the time, whereas in fact, an earlier return means thats not the case.

Nesting is good, it lets you see whats what. If somethings indented you know its in an if or a while block (for example).

Theres actually been some interesting research done into how programmers read code, and it actually has very little to do with the actual words you read, and has much more to do with colouring and being able to quickly identify syntax from the visual pattern without reading it explicitly.

Try changing your VS colour scheme and see how hard it is to read it until youre used to it (took me a few days to go from black on white to white on black - but now i love it)

Edit: If you think reducing nesting is good, because your methods have lots of nesting then you need to refactor because its too complicated. You should keep nesting down by good design, not cheating with confusing early returns.

Answer 7

Guard clauses or pre-conditions (as you can probably see) check to see if a certain condition is met and then breaks the flow of the program. They're great for places where you're really only interested in one outcome of an if statement. So rather than say:

if (something) {
    // a lot of indented code
}

You reverse the condition and break if that reversed condition is fulfilled

if (!something) return false; // or another value to show your other code the function did not execute

// all the code from before, save a lot of tabs

return is nowhere near as dirty as goto. It allows you to pass a value to show the rest of your code that the function couldn't run.

You'll see the best examples of where this can be applied in nested conditions:

if (something) {
    do-something();
    if (something-else) {
        do-another-thing();
    } else {
        do-something-else();
    }
}

vs:

if (!something) return;
do-something();

if (!something-else) return do-something-else();
do-another-thing();

You'll find few people arguing the first is cleaner but of course, it's completely subjective. Some programmers like to know what conditions something is operating under by indentation, while I'd much rather keep method flow linear.

I won't suggest for one moment that precons will change your life or get you laid but you might find your code just that little bit easier to read.

Answer 8

My idea is that the return "in the middle of a function" shouldn't be so "subjective". The reason is quite simple, take this code:

    function do_something( data ){

      if (!is_valid_data( data )) 
            return false;


       do_something_that_take_an_hour( data );

       istance = new object_with_very_painful_constructor( data );

          if ( istance is not valid ) {
               error_message( );
                return ;

          }
       connect_to_database ( );
       get_some_other_data( );
       return;
    }

Maybe the first "return" it's not SO intuitive, but that's really saving. There are too many "ideas" about clean codes, that simply need more practise to lose their "subjective" bad ideas.

Answer 9

A return in the middle of the method is not necessarily bad. It might be better to return immediately it it makes the intent of the code clearer. For example:

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

In this case, if _isDead is true, we can immediately get out of the method. It might be better to structure it this way instead:

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

I've picked this code from the refactoring catalog. This specific refactoring is called: Replace Nested Conditional with Guard Clauses.

Answer 10

The idea of only returning at the end of a function came back from the days before languages had support for exceptions. It enabled programs to rely on being able to put clean-up code at the end of a method, and then being sure it would be called and some other programmer wouldn't hide a return in the method that caused the cleanup code to be skipped. Skipped cleanup code could result in a memory or resource leak.

However, in a language that supports exceptions, it provides no such guarantees. In a language that supports exceptions, the execution of any statement or expression can cause a control flow that causes the method to end. This means clean-up must be done through using the finally or using keywords.

Anyway, I'm saying I think a lot of people quote the 'only return at the end of a method' guideline without understanding why it was ever a good thing to do, and that reducing nesting to improve readability is probably a better aim.

If methods are defined to do just one thing, which is usually a good idea, then this template works well:

void AMethod()
{
  check preconditions and return/throw/assert if wrong
  do the thing
}

If you've got deep nesting, maybe your function is trying to do too many things.

Answer 11

I prefer throwing an exception (that the same function catches) instead of having multiple returns. That's because you may way want to carry out some other operations or just clean up the memory allocated.

try {

if (!condition) throw -1;

. . .

if (!condition) throw -1;

. . .

}catch(int iErr) {

//clean up and return or just fall through

}

Answer 12

Multiple return points were a problem in C (and to a lesser extent C++) because they forced you to duplicate clean-up code before each of the return points. With garbage collection, the try | finally construct and using blocks, there's really no reason why you should be afraid of them.

Ultimately it comes down to what you and your colleagues find easier to read.

Answer 13

There are several good points made here, but multiple return points can be unreadable as well, if the method is very lengthy. That being said, if you're going to use multiple return points just make sure that your method is short, otherwise the readability bonus of multiple return points may be lost.

Answer 14

There are several advantages to this sort of coding but for me the big win is, if you can return quick you can improve the speed of your application. IE I know that because of Precondition X that I can return quickly with an error. This gets rid of the error cases first and reduces the complexity of your code. In a lot of cases because the cpu pipeline can be now be cleaner it can stop pipeline crashes or switches. Secondly if you are in a loop, breaking or returning out quickly can save you a lots of cpu. Some programmers use loop invariants to do this sort of quick exit but in this you can broke your cpu pipeline and even create memory seek problem and mean the the cpu needs to load from outside cache. But basically I think you should do what you intended, that is end the loop or function not create a complex code path just to implement some abstract notion of correct code. If the only tool you have is a hammer then everything looks like a nail.

Answer 15

In my opinion early return is fine if you are just returning void (or some useless return code you're never gonna check) and it might improve readability because you avoid nesting and at the same time you make explicit that your function is done.

If you are actually returning a returnValue - nesting is usually a better way to go cause you return your returnValue just in one place (at the end - duh), and it might make your code more maintainable in a whole lot of cases.

Answer 16

Many good reasons about how the code looks like. But what about results?

Let's take a look to some C# code and its IL compiled form:

using System;

public class Test {
    public static void Main(string[] args) {
     if (args.Length == 0) return;
     if ((args.Length+2)/3 == 5) return;
     Console.WriteLine("hey!!!");
    }
}

This simple snippet can be compiled. You can open the generated .exe file with ildasm and check what is the result. I won't post all the assembler thing but I'll describe the results.

The generated IL code does the following:

1. If the first condition is false, jumps to the code where the second is.
2. If it's true jumps to the last instruction. (Note: the last instruction is a return).
3. In the second condition the same happens after the result is calculated. Compare and: got to the Console.WriteLine if false or to the end if this is true.
4. Print the message and return.

So it seems that the code will jump to the end. What if we do a normal if with nested code?

using System;

public class Test {
    public static void Main(string[] args) {
     if (args.Length != 0 && (args.Length+2)/3 != 5) 
     {
      Console.WriteLine("hey!!!");
     }
    }
}

The results are quite similar in IL instructions. The difference is that before there were to jumps per condition: if false go to next piece of code, if true go to then end. And now the IL code flows better and has 3 jumps (the compiler optimized this a bit): 1. First jump: when Length is 0 to a part where the code jumps again (Third jump) to the end. 2. Second: in the middle of the second condition to avoid one instruction. 3. Third: if the second condition is false, jump to the end.

Anyway, the program counter will always jump.

Answer 17

Personally I prefer only 1 exit point. It's easy to accomplish if you keep your methods short and to the point, and it provides a predictable pattern for the next person who works on your code.

eg.

 bool PerformDefaultOperation()
 {
      bool succeeded = false;

      DataStructure defaultParameters;
      if ((defaultParameters = this.GetApplicationDefaults()) != null)
      {
           succeeded = this.DoSomething(defaultParameters);
      }

      return succeeded;
 }

This is also very useful if you just want to check the values of certain local variables within a function before it exits. All you need to do is place a breakpoint on the final return and you are guaranteed to hit it (unless an exception is thrown).