Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
5  
If you are checking only errors in the first 'if', you better not include 'else' because errors should not be thought as being part of the actual logic. –  Mert Jul 19 '12 at 13:41
1  
personally i'd be more worried about the function causing side-effects, but i guess this is just a poorly chosen example? –  jk. Jul 19 '12 at 14:21
1  
11  
To me, the first version is like dismissing all the students in the room who didn't finish their homework, and then once they are gone, saying to the rest of the students "Now if you did finish your homework...". It makes sense, but is unnecessary. Since the conditional is not really a conditional anymore, I tend to drop the else. –  NickC Jul 20 '12 at 6:05

17 Answers 17

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

share|improve this answer
3  
@pR0Ps has the same answer below and provides a code example as to why it ends up being a cleaner path to follow. +1 for the good answer. –  GlenH7 Jul 19 '12 at 16:12
2  
+1. Inappropriately avoiding guard blocks tends to cause arrow code. –  Brian Jul 19 '12 at 19:46

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.

It comes down to personal preference, 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. It might not make a difference in a small method, but with larger blocks of code, it could.

share|improve this answer
14  
Example 2 would be even easier to read if you got rid of the second else. –  briddums Jul 19 '12 at 17:17

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

share|improve this answer

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.

share|improve this answer
8  
+1 for "I don't like functions with several exit points" –  Corv1nus Jul 19 '12 at 14:56
9  
Of functions with multiple exit points and functions with a mutable result variable, the former seems to me by far the lesser of two evils. –  Jon Purdy Jul 19 '12 at 22:34

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.

share|improve this answer

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.

share|improve this answer

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

share|improve this answer
2  
If a method is so long that the second form is hard to understand, it is too long. –  kevin cline Jul 19 '12 at 14:38
7  
Your two cases are only equivalent if do something includes a return. If not, the second code will execute do somethingelse regardless of the if predicate which is not how the first block works. –  Adnan Jul 19 '12 at 17:37

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.

share|improve this answer

I dislike solution 1 because the 'else' clause is obsolete, and I don't like obsolete code.

share|improve this answer
2  
It may be unnecessary (depending on how the code is written) but else certainly isn't obsolete. –  Michael Kjörling Jul 20 '12 at 12:46

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.

share|improve this answer

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 less set of braces to care for.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
1  
Personally, I hope that I never have to work with your code. –  Michael Kjörling Jul 20 '12 at 12:50

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.

share|improve this answer
1  
The question is why do you prefer that style? –  William Shakespeare Jul 20 '12 at 7:48
1  
@Shadur We are not doing optimisation here, right? My point is to make my code easy to read and maintain, and personally I would pay that with the cost of one reference null check. –  tia Jul 20 '12 at 10:07
1  
@tia I like the spirit of your code. Instead of the evaluating the userName twice I'd capture the first evaluation in a variable and return that. –  Chuck Conway Jul 24 '12 at 19:22

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.