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

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 2

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 3

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 4

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

Answer 5

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 6

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 7

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 8

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 9

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");
}