Best practice on if/return

Question

I want to know what is considered better way of returning when I have if statement.

Example 1:

public bool MyFunction()
{
   // Get some string for this example
   string myString = GetString();

   if (myString == null)
   {
      return false;
   }
   else
   {
      myString = "Name " + myString;
      // Do something more here...
      return true;
   }
}

Example 2:

public bool MyFunction()
{
   // Get some string for this example
   string myString = GetString();

   if (myString == null)
   {
      return false;
   }

   myString = "Name " + myString;
   // Do something more here...
   return true;
}

As you can see in both examples function will return true/false but is it a good idea to put else statement like in first example or it is better to not put it?

Answer 1

Example 2 is known as guard block. It is better suited to return/throw exception early if something went wrong (wrong parameter or invalid state). In normal logic flow it is better to use Example 1

Answer 2

My personal style is to use the single if for guard blocks, and the if/else in the actual method processing code.

In this case, you're using the myString == null as a guard condition, so I would tend to use the single if pattern.

Consider code that's a little more complicated:

Example 1:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }
    else{
        //processing block
        myString = escapedString(myString);

        if (myString == "foo"){
            //some processing here
            return false;
        }
        else{
            myString = "Name " + myString;
            //other stuff
            return true;
        }
    }
}

Example 2:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }

    //processing block
    myString = escapedString(myString);

    if (myString == "foo"){
        //some processing here
        return false;
    }
    else{
        myString = "Name " + myString;
        //other stuff
        return true;
    }
}

In Example 1, both the guard and the rest of the method are in the if/else form. Compare that to Example 2, where the guard block is in the single if form, while the rest of the method uses the if/else form. Personally, I find example 2 easier to understand, while example 1 looks messy and over-indented.

Note that this is a contrived example and that you could use else if statements to clean it up, but I'm aiming to show the difference between guard blocks and the actual function processing code.

A decent compiler should generate the same output for both of them anyway. The only reason to use one or the other is personal preference or to conform to the style of the existing code.

Answer 3

personally, I prefer the second method. I feel as though it's shorter, has less indentation, and is easier to read.

Answer 4

My personal practice is the following:

• I don't like functions with several exit points, I found it hard to maintain and follow, code modifications sometimes break the internal logic because it is inherently a bit sloppy. When it is a complex calculation, I create a return value at the start and return it at the end. This forces me to carefully follow each if-else, switch, etc paths, set the value correctly at the proper locations. I also spend a bit of time on deciding whether to set a default return value, or leave it uninitialized at the start. This method also helps when the logic or the return value type or meaning changes.

For example:

public bool myFunction()
{
   // First parameter loading
   String myString = getString();

   // Location of "quick exits", see the second example
   // ...

   // declaration of external resources that MUST be released whatever happens
   // ...

   // the return variable (should think about giving it a default value or not) 
   // if you have no default value, declare it final! You will get compiler 
   // error when you try to set it multiple times or leave uninitialized!
   bool didSomething = false;

   try {
     if (myString != null)
     {
       myString = "Name " + myString;
       // Do something more here...

       didSomething = true;
     } else {
       // get other parameters and data
       if ( other conditions apply ) {
         // do something else
         didSomething = true;
       }
     }

     // Edit: previously forgot the most important advantage of this version
     // *** HOUSEKEEPING!!! ***

   } finally {

     // this is the common place to release all resources, reset all state variables

     // Yes, if you use try-finally, you will get here from any internal returns too.
     // As I said, it is only my taste that I like to have one, straightforward path 
     // leading here, and this works even if you don't use the try-finally version.

   }

   return didSomething;
}

• The only one exception: "quick exit" at the start (or in rare cases, inside the process). If the real calculation logic can't handle a certain combination of input parameters and internal states, or has an easy solution without running the algorithm, it does not help to have all the code encapsulated in (sometimes deep) if blocks. This is an "exceptional state", not part of the core logic, so I must get out of the calculation as soon as I have detected. In this case there is no else branch, in normal conditions the execution simply goes on. (Of course, "exceptional state" is better expressed by throwing an exception, but sometimes it is an overkill.)

For example:

public bool myFunction()
{
   String myString = getString();

   if (null == myString)
   {
     // there is nothing to do if myString is null
     return false;
   } 

   myString = "Name " + myString;
   // Do something more here...

   // not using return value variable now, because the operation is straightforward.
   // if the operation is complex, use the variable as the previous example.

   return true;
}

The "one exit" rule also helps when the calculation requires external resources that you have to release, or states that you have to reset before leaving the function; sometimes they are added later during development. With multiple exits inside the algorithm, it is much harder to extend all branches properly. (And if exceptions may occur, the release/reset should be put in a finally block as well, to avoid side effects in rare exceptional cases...).

Your case seems to fall into the "quick exit before real work" category, and I would write it like your Example 2 version.

Answer 5

I prefer to employ guard blocks where ever I can for two reasons:

1. They allow a quick exit given some specific condition.
2. The remove the necessity for complex and un-necessary if statements later in the code.

Generally speaking I prefer to see methods where the core functionality of the method is clear and minimal. Guard blocks help to visually make this happen.

Answer 6

I like the "Fall Through" approach:

public bool MyFunction()
{
   string myString = GetString();

   if (myString != null)
   {
     myString = "Name " + myString;
     return true;
    }
    return false;
}

The action has a specific condition, anything else is just the default "fall through" return.

Answer 7

If I have a single if condition I wouldnt spend too much time ruminating about the style. But if I have multiple guard conditions I would prefer style2

Picuture this. Assume that the tests are complex and you really dont want to tie them into a single if-ORed condition to avoid complexity:

//Style1
if (this1 != Right)
{ 
    return;
}
else if(this2 != right2)
{
    return;
}
else if(this3 != right2)
{
    return;
}
else
{
    //everything is right
    //do something
    return;
}

versus

//Style 2
if (this1 != Right)
{ 
   return;
}
if(this2 != right2)
{
    return;
}
if(this3 != right2)
{
    return;
}


//everything is right
//do something
return;

Here there are two main advantages

1. You are separating the code in a single function into two visually logcal blocks : an upper block of validations(guard conditions) and a lower block of runnable code.

2. If you have to add/remove one condition , you reduce your chances of messing up the entire if-elseif-else ladder.

Another minor advantage is you have one fewer set of braces to care for.

Answer 8

This should be a matter of which sounds better.

If condition
  do something
else
  do somethingelse

expresses itself better then

if condition
  do something
do somethingelse

for small methods it wont be much of a difference, but for larger compound methods it can make it harder to understand as it wont be properly seperate

Answer 9

I find example 1 irritating, because the missing return statement at the end of a value-returning function immediately triggers the "Wait, there's something wrong"-flag. So in cases like this, I would go with example 2.

However, usually there is more involved, depending on the purpose of the function, e.g. error handling, logging, etc. So I'm with Lorand Kedves' answer on this, and usually have one exit point at the end, even at the cost of an additional flag variable. It makes maintenance and later extensions easier in most cases.

Answer 10

When your if statement always returns then there is no reason to use an else for the remaining code in the function. Doing so adds extra lines and extra indentation. Adding unnecessary code makes it harder to read and as everyone knows Reading code is Hard.

Answer 11

In your example, the else is obviously unnecessary, BUT ...

When skimming quickly through lines of code, one's eyes typically look at the braces and the indentations to get an idea of the flow of code; before they actually settle on the code itself. Therefore, writing the else, and putting braces and indentation around the second block of code actually makes it quicker for the reader to see that this is an "A or B" situation, where one block or the other will run. That is, the reader sees the braces and indentation BEFORE they see the return after the initial if.

In my opinion, this is one of those rare cases where adding something redundant to the code actually makes it MORE readable, not less.

Answer 12

I'd prefer example 2 because it's immediately obvious that the function returns something. But even more than that, I'd prefer to return from one place, like this:

public bool MyFunction()
{
    bool result = false;

    string myString = GetString();

    if (myString != nil) {
        myString = "Name " + myString;

        result = true;
    }

    return result;
}

With this style I can:

1. See right away that I'm returning something.

2. Catch the result of every invocation of the function with just one breakpoint.

Answer 13

My personal style tends to go

function doQuery(string) {
    if (!query(string)) {
        query("ABORT");
        return false;
    } // else
    if(!anotherquery(string) {
        query("ABORT");
        return false;
    } // else
    return true;
}

Using the commented-out else statements to indicate the program flow and keep it readable, but avoiding massive indents that can easily reach across the screen if a lot of steps are involved.

Answer 14

I would write

public bool SetUserId(int id)
{
   // Get user name from id
   string userName = GetNameById(id);

   if (userName != null)
   {
       // update local ID and userName
       _id = id;
       _userNameLabelText = "Name: " + userName;
   }

   return userName != null;
}

I prefer this style because it is most obvious that I would return userName != null.

Answer 15

My rule of thumb is to either do a if-return without an else (mostly for errors) or do a full if-else-if chain over all possibilities.

This way I avoid trying to be too fancy with my branching, since whenever I end up doing that my I encode application logic into my ifs, making them harder to maintain (For example, if an enum OK or ERR and I write all my ifs taking advantage of the fact that !OK <-> ERR it becomes a pain in the ass to add a third option to the enum next time.)

In your case, for example, I would use a plain if, since "return if null" is sure to be a common pattern and its not likely that you will need to worry about a third possibility (in addition to null/not null) in the future.

However, if the test was something that did a more in-dept inspection over the data I would err towards a full if-else instead.

Answer 16

I think there are a lot of pitfalls to Example 2, that down the road could lead to unintended code. 1st off the focus here is based on logic surrounding the 'myString' variable. Therefore to be explicit, all testing of conditions should happen in a code block accounting for known and default/unknown logic.

What if later on code was introduced unintentionally to example 2 that altered the ouput significantly:

   if (myString == null)
   {
      return false;
   }

   //add some v1 update code here...
   myString = "And the winner is: ";
   //add some v2 update code here...
   //buried in v2 updates the following line was added
   myString = null;
   //add some v3 update code here...
   //Well technically this should not be hit because myString = null
   //but we already passed that logic
   myString = "Name " + myString;
   // Do something more here...
   return true;

I think with the else block immediately following the check for a null would have made programmers that added the enhancements to the future versions add all the logic together because now we have a string of logic that was unintended for the original rule (returning if the value is null).

I lend my belief in this heavily to some of the C# guidlines on Codeplex (link to that here: http://csharpguidelines.codeplex.com/) that state the following:

"Add a descriptive comment if the default block (else) is supposed to be empty. Moreover, if that block is not supposed to be reached throw an InvalidOperationException to detect future changes that may fall through the existing cases. This ensures better code, because all paths the code can travel has been thought about."

I think it is good programming practice when using logic blocks like this to always have a default block added (if-else, case:default) to account for all code paths explicitly and not to leave code open to unintended logic consequences.