Avoid return statement in the middle - horror or not?

[Mladen Jankovic]

Robert.C.Cartaino wrote:

Writing code that depends on order of evaluation is bad programming practice

If order of evaluation is not defined by the specification of the language, which is not true for logical operators in C/C++. According to your statement one should not write something like this:

if( node && node->value )
node->value->Do();

Instead you need to have another if for this simple operation even if the previous example is perfectly legal and clean and the desired behavior is guaranteed by the language specification. Well I guess that the only bad programming practice here is that someone introduces unneeded code because he doesn't trust/know language.

[Genetic Algorithm Library]

[PIEBALDconsult]

Indeed. And should we question the order/precedence of the other operators as well? Can we trust 3 + 4 * 5 to return 23? or might it return 35 once in a while? I'm all for adding parentheses for clarity, but I don't get that confused with necessity.

[Ben Fair]

Yes, he just inverted the boolean logic and made it worse in my opinion. if(!FlagA || !FlagB) is essentially the same as if(FlagA && FlagB) but I'd rather see the latter and that's how I'd do it in my code.

Keep It Simple Stupid! (KISS)

[geoffs]

It's not. Your code was how I would have done it...

[User 167261]

it all depends on how much coffe i've had for the day...

do or do not, there is no try

[PIEBALDconsult]

Drinking coffee leads to good code. Drinking "energy drinks" leads to poor code.

[FatBuddha]

Assuming that there is more processing going on than this example indicates, I would use a do-while-false loop:

do{
if(!flagA)break;
if(!flagB)break;
if(!flagC)break;
if(!PromptUser())break;
}while(0);

DoOtherThing();

This is the sort of thing that I routinely do with COM programming where I need to check the success of pretty much every single method call. It means that I don't have a crazy amount of indenting, and it's much cleaner for me or somebody else to understand and modify later. I can highly recommend it.

None

[rfidel]

Not sure if this made one of the previous replies, but what I found works best, and is easy to follow, is to iteratively step thru what I do not want, making use "else if" statements. After weeding thru all the crap, then what I am left with, is (usually) what I want. if(!flagA) { DoOtherThing(); } else if(!flagB) { DoOtherThing(); } else if(!flagC) { DoOtherThing(); } else if(!PromtUser()) { DoOtherThing(); } else { DoSomething(); }

[PIEBALDconsult]

That's simply the first snippet inverted and made even uglier. It's still a maintenance nightmare.

[PIEBALDconsult]

Where's the call to DoSomething? I'm pretty sure that's not applicable to the original post.

[FatBuddha]

Doh - I was looking at the second code listing. Assuming that there is lots of other stuff going on between the flag tests, and there is a need for clean-up code (thus don't want to use return), I'd use the same system with a flag:

//Figure out what to do...
bool doSomething = false;
do{
if(!flagA)break;
if(!flagB)break;
if(!flagC)break;
if(!PromptUser())break;
doSomething = true;
}while(0);

//...then do it
if(doSomething)
DoSomething();
else
DoOtherThing();

None

[PIEBALDconsult]

Still looks like a potentially infinite loop.

[Manish K Agarwal]

I usualy perfer "goto cleanup" statement (where cleanup is lable inside the function) instead of making code hard to read.

Manish Agarwal manish.k.agarwal @ gmail DOT com

[Marcello Faga]

Georgi, I agree with you. "Return in the middle" is an horror! X|

**************************** Strong congruence for strong people; with a compatible behaviour. For a semantical way of life.

[FatBuddha]

I can assure you that it isn't! This technique is usually called a do-once block, and commented as such to make it really obvious. Look again at just the 'loop' bit and see if you think that this block of code will still run more than once:

do{//once

}while(0);

What you basically end up with, is a block of code that you can jump to the end of early with a 'break', without having to resort to a 'goto'. That block of code isn't a loop, as it will never run more than once. Get it?

None

[PIEBALDconsult]

:doh: Oh, right, that's a 0, my mistake. (Though I still say it looks like an infinite loop.) And that technique is a horror unto itself -- a Weasel-Goto. X|

[PIEBALDconsult]

::Shudder::

[John M Drescher]

That is pretty much what I do or just break up the function into smaller parts if there are too many levels of conditionals.

John

[riced]

I would tend do follow Michael Jackson's advice: "At this point you might be tempted to introduce a flag. Avoid such Satanic practices." I quote from memory not having read it in the last 10 years. Use of 'flags' to control program flow is often a bad code smell.

[Thomas Weller 0]

While having many return statements might be questionable, I think readability is much more important. So I would always allow for multiple return statements to avoid deep nesting. Deep nesting makes the code going out of the right side of the display and it is also harder to understand. Over time, this can become a big maintainability issue. In your (first) example, we can do without a single return. We can be explicit and well structured at the same time like so:

bool needToDoSomething = false;

if (FlagA && FlagB && FlagC)
{
needToDoSomething = PromptUser();
}

if (needToDoSomething)
{
DoSomething();
}
else
{
DoOtherThing();
}

You should always write your code such that a potential reader can quickly understand your original intention without the need to scroll in any direction. Another issue arises with the code above when it comes to unit testing and code coverage: We cannot setup code coverage for every single condition in if (FlagA && FlagB && FlagC) - we can do this only for the whole line. If we want to be accurate with this and setup an individual test case for every single condition, we can only do this by using a waterfall-like coding style:

bool needToDoSomething = false;

if (!needToDoSomething)
needToDoSomething |= FlagA;
if (!needToDoSomething)
needToDoSomething |= FlagB;
if (!needToDoSomething)
needToDoSomething |= FlagC;

if (needToDoSomething)
needToDoSomething = PromptUser();

if (needToDoSomething)
{
DoSomething();
}
else
{
DoOtherThing();
}

Probably not the most elegant solution and surely not the shortest one, but it is easy to read/understand and it has a much better testability than the first example. And this in my view is much more important than any other argument. Regards Thomas

www.thomas-weller.de Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software.