Removing else blocks

[qazmlp]

I have an 'if else' statement block like this:

if( inputArg != VALID_VALUE )
{
return error ;
}
else
{
// Do some major operation
return success ;
}

I am thinking of changing it to something like this:
if( inputArg != VALID_VALUE )
{
return error ;
}

// Do some major operation
return success ;


Which style do you recommend? Why?

 

[John Harrison]

I have an 'if else' statement block like this:

if( inputArg != VALID_VALUE )
{
return error ;
}
else
{
// Do some major operation
return success ;
}

I am thinking of changing it to something like this:
if( inputArg != VALID_VALUE )
{
return error ;
}

// Do some major operation
return success ;


Which style do you recommend? Why?

The second.

Because exceptional processing (like error handling) should not be given
equal weight with normal processing. By having the error handling part of
you function in a separate if statement from the main part of the function
you are demonstrating that both pieces of code don't have equal importance.
Having both parts of the code in an if else statement implies some sort of
equivalence.

john

 

[Thore Karlsen]

I have an 'if else' statement block like this:

if( inputArg != VALID_VALUE )
{
return error ;
}
else
{
// Do some major operation
return success ;
}

I am thinking of changing it to something like this:
if( inputArg != VALID_VALUE )
{
return error ;
}

// Do some major operation
return success ;


Which style do you recommend? Why?

I typically use the second style, because it avoids needless
indentation. I prefer to bail out immediately if there's a problem,
instead of having a whole mess with multiple if/elses.

There are those that think that a function should only have one exit,
and it's quite a religious issue. Personally, I think that rule just
causes longer, more unreadable, and more unmaintanable code. I keep my
functions short and simple, so the single exit rule is not as beneficial
as perhaps it would be in larger functions.

 

[John Harrison]

There are those that think that a function should only have one exit,

and it's quite a religious issue.

This is structured programming, as exemplified by Pascal. Every function
should have one entrance point and one exit point. Everyone accepts the
first part, very few the second part, especially if they have done real
world programming where you do have to think about error handling.

C++ exceptions provide an even clearer way to bail out of a function upon
detecting an error. In code like this the use of else looks perverse to me.

if (some error)
{
throw some exception;
}
else
{
carry on as normal;
}

john

 

[Fraser Ross]

The first style might result in a 'function should return a value' warning
even though it always does. I would use the second style for that reason.

Fraesr.

 

[Jeff Schwab]

I have an 'if else' statement block like this:

if( inputArg != VALID_VALUE )
{
return error ;
}
else
{
// Do some major operation
return success ;
}

I am thinking of changing it to something like this:
if( inputArg != VALID_VALUE )
{
return error ;
}

// Do some major operation
return success ;


Which style do you recommend? Why?

When I'm checking for something I expect to happen in the normal course
of the program, my preference is to maintain a single point of exit:

Status function( Input const& input ) {

Status status; // To be returned.

if( ! valid( input ) ) {
status = error;
}
else { // Input is valid.
status = success;
}

return status;
}

The biggest reason for this is that it guarantees a place in each
function where I can check the function's value (and any other program
state) immediately before the function returns. This isn't something I
did in school; it took a couple of years of "real world" debugging to
appreciate.

If a run-time check uncovers a truly exceptional condition, I prefer to
throw an exception. "Truly exceptional" means (to me) that it may not
be feasible to recover from the error. If one of my functions is called
incorrectly, I typically throw, since I no longer trust the immediately
higher layer of the program.

template< typename N >
N reciprocal( N const& n ) {

if( n == 0 ) {
throw Exceptions:ivision_by_zero(
"attempt to find reciprocal of zero" );
}

N result = 1/n;

return result;
}

In most of these cases I actually use an ASSERT macro to do the "throw
if check fails." I know Bjarne recommends using a template for
Assertions, but I like my error messages to include the failed
expression, as well as the file name and line number.

As Thore said, this is a pretty religious issue. Bright people have
suprisingly different philosophies. The conversation seems to be along
these lines:

A: I like to keep a single point of exit.
B: That makes the code less readable.
A: But much more debuggable.
B: No, you're wrong.
A: No, YOU are wrong.
B: Taste my wrath!

[ Mild flames ensue. ]

Whatever you do, you're likely to upset a few folks. Pick whatever
makes sense to you and go with it. If you change your mind later, at
least you'll know *why* the way you've been doing it is wrong. Then you
can be religious about it, too.

 

[Magnus]

When I'm checking for something I expect to happen in the normal course
of the program, my preference is to maintain a single point of exit:

[snip]

In certain situations, and maybe in the long run, might not this give you a
performance penalty, in contrast to exiting the function once an error is
discovered ?

- Magnus

 

[Jeff Schwab]

When I'm checking for something I expect to happen in the normal course
of the program, my preference is to maintain a single point of exit:

[snip]

In certain situations, and maybe in the long run, might not this give you a
performance penalty, in contrast to exiting the function once an error is
discovered ?

If anything, I would guess that a block of code with only one entrance
and one exit would be more amenable (than its alternative) to
optimization by a compiler. [NB: I am not an implementer.] The only
potential hit to performance I see is the construction of the return
value before the function is ready to initialize it. If the type of the
return value has no user-defined constructor, this isn't an issue.

On the first draft of a program, I believe it is more important to
establish semantically correct behavior, than it is to meet the
program's eventual performance requirements. Once confidence has been
established in the correctness of the program, profiling can be used to
help target optimization effectively. As optimizations are made, it is
vital to maintain the conceptual integrity of the program. I can't
imagine a program's performance bottleneck being the direct result of
maintaining a single point of exit in some block of code, but even if I
ever saw such a thing, allowing multiple return points as part of the
normal flow of the program would be a last resort.

Of course, there is also the old adage that time spent debugging is time
*not* spent optimizing.

 

[Daniel T.]

I have an 'if else' statement block like this:

if( inputArg != VALID_VALUE )
{
return error ;
}
else
{
// Do some major operation
return success ;
}

I am thinking of changing it to something like this:
if( inputArg != VALID_VALUE )
{
return error ;
}

// Do some major operation
return success ;


Which style do you recommend? Why?

The style I like best would be:

result = error;
if ( inputArg == VALID_VALUE )
{
// do some major operation
result = success;
}
return result;

In general, I don't use returns to indicate success/failure though.

I'm not a big fan of having multiple returns in a single function.
Debuging the code is more difficult for me.

 

[Brandon Mitchell]

As a general rule, the optimizer has difficulty operating on functions
with more than one 'return' statement. This has led to the 'religious
adherence' to this rule. However, those that prefer the simplified
expressions of the your second example, undoubtedly either a) don't mind
sacrificing (sp?) the optimizer on this function or b) don't realize
that this is happening. Personally, I prefer to keep with the the 'one
entrance, one exit' philosophy.