What is the cleanest way to write this if..then logic?

Question

They both do the same thing. Is one way better? Obviously if I write the code I'll know what I did, but how about someone else reading it?

if (!String.IsNullOrEmpty(returnUrl))
{
    return Redirect(returnUrl);
}
return RedirectToAction("Open", "ServiceCall");

OR

if (!String.IsNullOrEmpty(returnUrl))
{
   return Redirect(returnUrl);
}
else
{
    return RedirectToAction("Open", "ServiceCall");
}

Answer 1

the second way is better, no confusion about what you mean...

Answer 2

return String.IsNullOrEmpty(returnUrl) ? 
   RedirectToAction("Open", "ServiceCall") : 
   Redirect(returnUrl);

I prefer that.

Or the alternative:

return String.IsNullOrEmpty(returnUrl)  
   ? RedirectToAction("Open", "ServiceCall")  
   : Redirect(returnUrl);

Answer 3

I think this is a fairly small matter of style. I'd argue that your two samples are equally readable.

I prefer the former, but other people prefer only one exit point from a function and would probably suggest something like:

if (!String.IsNullOrEmpty(returnUrl))
{
   result = Redirect(returnUrl);
}
else
{
    result = RedirectToAction("Open", "ServiceCall");
}

return result;

Answer 4

I like the first example because it's more obvious that this excerpt will return. If both returns are in indented blocks, it takes just a little more mental effort to tell.

Answer 5

I believe it's better to remove the not (negation) and get the positive assertion first:

if (String.IsNullOrEmpty(returnUrl))
{
   return RedirectToAction("Open", "ServiceCall");
}
else
{
    return Redirect(returnUrl);
}

-or-

// Andrew Rollings solution
return String.IsNullOrEmpty(returnUrl) ? 
                    RedirectToAction("Open", "ServiceCall") : 
                    Redirect(returnUrl);

Answer 6

How about:

if (!String.IsNullOrEmpty(returnUrl))
    return Redirect(returnUrl);
else
    return RedirectToAction("Open", "ServiceCall");

Answer 7

One style issue:

if (String.IsNullOrEmpty(returnUrl))
{
    return RedirectToAction("Open", "ServiceCall");
}
return Redirect(returnUrl);

When you cancel out the double negation it reads a whole lot better, no matter which brace style you choose. Code that reads better is always best ;)

Answer 8

Don't think there's a better way... it depend on each developer. That's why before starting a project you should decide what is the coding standard ...

Answer 9

Eliminate the 'double-negative' and use the fully-expanded style. I am sure most compilers now can suitably optimize the code on your behalf, so no reason to take short-cuts for readability.

Answer 10

I like the first better.

if ( ) 
{
    return ... 
}
return

For me it reads like a "default" you can chain more conditions but at the end there is a default.

Of course it is a matter of style.

Additional question.

Is it C# style to put the square brackets and in a single line?

if ( ) 
{
}
else
{
}

I have seen this permeated into Java code samples in SO, and I'm wondering this is the root cause.

EDIT

@Owen. I mean, Is it C# style using this form?

if ()  
{
    code ... 
}
else
{
    code...
}

Rather than this ( which would be Java preffered )

if ( ) { 
    code ... 
} else { 
   code ...
}

I have had some arguments in the past about this, but most of the times, only with people that come from C# background.

Answer 11

The first one. You don't need any else if you are returning a value inside the if statement. So:

if (!String.IsNullOrEmpty(returnUrl))
    return Redirect(returnUrl);
return RedirectToAction("Open", "ServiceCall");

Answer 12

If it's the entirety of the method then I'd say the second (using the else) is a bit more elegant. If you have preceding code or (especially) much more code before the return in the else case, I'd say it's better not to put the else. Keeps from code becoming too indented.

i.e. either:

void myfunc()
{
    if (!String.IsNullOrEmpty(returnUrl))
    {
        return Redirect(returnUrl);
    }
    else
    {
        return RedirectToAction("Open", "ServiceCall");
    }
}

or

void myfunc()
{
    // ... maybe some code here ...

    if(!String.IsNullOrEmpty(returnUrl))
    {
       return Redirect(returnUrl);
    }

    // ... a bunch of other code ...

    return RedirectToAction("Open", "ServiceCall");
}

Answer 13

As here, but more readable:

return
  string.isNullorEmpty(returnUrl) ? 
     RedirectToAction("Open", "ServiceCall") :
     Redirect(returnUrl);

This way works, is readable and non-redundant.

Answer 14

From a maintenance perspective I prefer the first version. I've seen less errors with a consistent "get out early" style.

The ternary operator (?:) is OK, but only if it is very unlikely that new code will be inserted before the 2nd return statement. Otherwise when the time comes to add something new into the 2nd code path, the ternary statement has to go back to being an if/else block anyway.

Answer 15

When I have only one line sometimes I compact the if statement like this:

if(String.IsNullOrEmpty(returnUrl)) { 
  return RedirectToAction("Open", "ServiceCall"); 
}
else{ return Redirect(returnUrl); }

Although when I look at it, Andrew Rollings might have the best solution although I had never thought of using it until today.

Answer 16

LFSR's "else" solution is the most maintainable and readable.

Using a separate else clause allows logic to be added without inadvertently changing the flow. And it's bad enough having multiple exit points without hiding two of them in a single ternary operator statement!

Answer 17

This code feels messy because of the unavoidable double-negative logic (and shuffling things around isn't going to clear it up). Whichever arrangement you use, I think you should add some comments so that the reader doesn't need to do a double-take:

if (!String.IsNullOrEmpty(returnUrl))
{
  // hooray, we have a URL
  return Redirect(returnUrl);
}
else
{
  // no url, go to the default place
  return RedirectToAction("Open", "ServiceCall");
}