Control Flow via Return vs. If/Else

Question

Which one is better (implicit control flow via return or control flow via if) -- see below. Please explain what you see as advantage/disadvantage to either one. I like option A because it's less code.

Flow via Return:

public ActionResult Edit(MyClass class)
{
    if (!class.Editable)
       return null;

    class.Update();
    return View();
}

Flow via If/Else:

public ActionResult Edit(MyClass class)
{
    if (class.Editable)
    {
       class.Update();
       return View();
    }
    else
    {
       return null;
    }
}

Answer 1

There's not much difference in this specific example, but in general I like the first approach because it uses a guard clause to return early. If you start adding nested conditions to the second approach you'll see that your code readability will suffer. Guard clauses can go a long way toward reducing the nesting depth, and really enhance the readability of your code.

Answer 2

I personally like the if/else approach. For one, your if statement is a positive, not a negative, making it easier to read. For two, you've encapsulated the conditions in curly brackets, and I'm a fan of that style.

Anyway, it's much easier to follow what's going on in the second than in the first. And that always wins in my book.

Answer 3

I prefer the second approach for the sake of readability and maintainability. Readability because it just reads 'cleaner' to me than the first approach, and maintainability because I don't have to worry about adding curly braces if I need to modify the if or else clauses. Further, the first approach is only 7 characters less than the second approach if you don't include new lines, which hardly seems a justification for choosing the first over the second.

That said, I actually prefer this:

public ActionResult Edit(MyClass class)
{
    ActionResult rv = null;
    if (class.Editable)
    {
        class.Update();
        rv = View();
    }
    return rv;
}

It's more code, but I can now set a single breakpoint on the return statement to inspect the value being returned instead of having to set two breakpoints to do the same in the two choices you offered.

Answer 4

Both of those statements are controlling flow via an if statement. It's just a matter of how you handle the condition.

I'm always on the fence when it comes to writing logic statements like this. Part of me likes the first option because it's a little less code. The other part of me likes the second option because it's much easier to follow the flow of logic. With the first option, it's easy to miss the return statement which can lead to manageability issues in the future.

...and for that reason, the second option always wins in my book. It's better to write code that is easier to read and maintain than try to take shortcuts.

Answer 5

I would prefer the one I identify as being the one which EXECUTES less code.
If it is more common to class.Editable being false then I'd go for A.

But this example does not give much of an advantage in either case.

In any given situation a developer should analyze the input and adjust the code to be optimized on the most common input data.

EDIT:
To clarify:
By executes less code I in reality mean is most efficient...

Answer 6

Exit early - I prefer to see all the conditions that will cause the method to exit without doing much up front. I avoid else statements if I can at all avoid it.

This is actually a fairly prominent school of thought among the Code Contracts crowd.

Answer 7

under these circumstances, I would go with option A. In this case you are doing your input validation and then preventing execution of the rest of the code if the input is not valid (not editable). This keeps the entire body of the function out of a big if/else statement and makes it more readable.

However, I would also consider raising an exception rather than retuning a null - that is assuming that passing in a non-editable object into an "edit" function isn't a normal occurrence.

Answer 8

They are both valid options, and one isn't necessarily any better than the other. Which one you choose is, ultimately, personal preference. Yes, Option A results in slightly less code, but overall they are pretty much equal.

In both cases you are controlling flow via an if and a return. It's really a question of how you prefer to see your boolean logic - negative or positive?

Is ActionResult an enum or a base class? If it's an enum, why are you returning null when Edit returns what appears to be an enum? Wouldn't it be cleaner simply to return an ActionResult value that indicates no action was taken because the object wasn't in an editable state?

Answer 9

I prefer if/else, too. Legibility, readability and maintainability stands above anything else, for me.

Answer 10

First option, using return, is better, because:

1. you have a place to put all guards and preconditions, near your asserts and all that stuff.
2. for me, it's easier to think "let's see all that can be wrong, and return. From this point, I have everything I need and I am on the Happy Path
3. if you do use the if / else approach, you have all code in that method / function indented. Add other if, or for, and things start to get funny

One proponent of this method (return) is Marcus Zarra, in the Cocoa is my Girlfriend coding style

Answer 11

I prefer the first option, provided the case you check is a guard/precondition that needs to be met for the method call to be valid. Although you could argue if you should return null, or throw an (Argument)Exception. When a class isn't editable, should it really be a parameter for this method?

Maybe a better option would be to create an IEditable interface and implementing this on the class you're passing an instance of right now.

Answer 12

I also prefer option 1. For me, it reads better like a book. Also I'm always pained by there not being a return at the end of option 2.