Explicit or implicit execution control statement use

Question

I sometimes use

if (this._currentToolForeColor.HasValue)
    return this._currentToolForeColor.Value;
else
    throw new InvalidOperationException();

other times I use

if (this._currentToolForeColor.HasValue)
    return this._currentToolForeColor.Value;
throw new InvalidOperationException();

The two are equivalent, I know, but I am not sure which is the best and why.

This goes even further as you can use other execution-control statements such as brake or continue :

while(something)
{
    if(condition)
    {
        DoThis();
        continue;
    }
    else
        break;
}

versus

while(something)
{
    if(condition)
    {
        DoThis();
        continue;
    }
    break;
}

EDIT 1 : Yes the loop example(s) suck because they are synthetic (i.e.: made up for this question) unlike the first which is practical.

Answer 1

Fail early.

Do your validation at the top of the method, and if you get past it, you can use the values in (relative) safety.

public void SomeFunction(object someParameter)
{
    if (someParameter == null) throw new ArgumentNullException("someParameter", "Value cannot be null");

    // Use the parameter here for stuff
    SomeOtherFunction(someParameter, Whichway.Up);
}

The same goes for the return statement. If your method is done, don't wrap stuff in big if statements, just return and get out of there.

See reducing cyclomatic complexity for other examples of this concept. Notice in my example there is only one code path, it's just a matter of how far along you can get with given parameters. Nested ifs and braces (even if you don't use the braces, as in your example) all have a greater impact on readability than the difference between your examples.

Answer 2

I guess that depends on your preferences. I personally prefer the first options since it is less verbose. Luckily Resharper and I agree on this, so it is easy to let R# change the code for the shorter version.

Answer 3

For a better and cleaner code, I think it is better to choose always this form:

if(condition)
{
   //do stuff
}
else
{
   //do stuff
}

..and so on.

It's just a matter of "taste", but even if you have more rows of code, this is clearly readable, because you don't have to interpret nor understand, you can follow the code flow with your finger easily. So, for me, always the else even if a return prevents fallback into following code, always brackets event if you have a one-row if/else and so on.

As far as I can remember, this is also suggested in Framework Design Guidelines, very easy book with lot of "do this" and "don't do this" guidelines.

Answer 4

I think in this case readibility is the main concern.

If you find yourself scrolling up ad down in the same method, or losing track of execution flow, it should probably be rewritten. break in the above indicates to me that the while condition was not sufficiant check.

It has its uses, but be careful, you dont want some coment appearing from another developer in the team saying

THERE BE DRAGONS HERE!!!

Answer 5

Resharper will recommend that you use this:

if( someCondition )
   return foo;

throw new InvalidArgumentException();

But, does that mean that it is therefore better ? I don't know. Personally, I find the following solution more explicit, and therefore I like it more. But that is just a personal choice.

if( someConditiion )
{
   return foo;
}
else
{
   throw new ....
}

Regarding your 'while' examples. IMHO, both your code examples suck. Why not write it like this:

while( something )
{
    if( !condition ) 
        break;

    DoStuff();
}

Answer 6

First and second options are equivalent to the compiler. But to a human reading the code, the first option is definitely more clear about the intent, and easier to read.

There are limits to what a human reader can absorb from thousands of lines of text designed primarily to function, not to convey meaning. Every tiny effort that makes the task easier is good.

Answer 7

while(something) {
    if(condition)
    {
        DoThis();
        continue;
    }
    else
        break; }

Is the same as the following:

while(something && condition)
{
     DoThis();
}

To your question: It is always better to be more explicit how the control flows even if it looks redundant (e.g. superfluous else connections are optimized away anyway). So if (cond) return x else throw y is good.

But in case of of error checking for method arguments you should do all checks at the start of the method, so you don't need any else's here.