Avoid return statement in the middle - horror or not?

[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.

[Thomas Weller 0]

Nah, goto is a horror itself. If you have the need for it, then you can be sure that there is something seriously wrong in your design... :( Regards Thomas (Btw: Does anybody know why it's still there at all in C#?)

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.

[PIEBALDconsult]

When you do need a goto (and I haven't since I quit BASIC) it's best to just use it. C# -- for switch statements.

[PIEBALDconsult]

Thomas Weller wrote:

but it is easy to read/understand

No it isn't, I can barely figure it out. Shouldn't needToDoSomething start off true and get changed to false if any test fails?

bool needToDoSomething = true;
if (needToDoSomething) needToDoSomething = FlagA;
if (needToDoSomething) needToDoSomething = FlagB;
if (needToDoSomething) needToDoSomething = FlagC;
if (needToDoSomething) needToDoSomething = PromptUser();

(Dang, C# doesn't have a &&= operator, that'd be useful here.) Maybe it's a language issue, C# has an actual boolean type, C/C++ doesn't, which are you using?

[Lutoslaw]

FatBuddha wrote:

bool doSomething = false; do{ if(!flagA)break; if(!flagB)break; if(!flagC)break; if(!PromptUser())break; doSomething = true; }while(0);

What about

if (!(
!flagA
|| !flagB
|| !flagC
|| !PromptUser()))
DoSomething();
else
DoOtherThing();

Greetings - Gajatko Portable.NET is part of DotGNU, a project to build a complete Free Software replacement for .NET - a system that truly belongs to the developers.

[PIEBALDconsult]

Still has all the needless negation; the version I posted earlier is still the cleanest, easiest to read, easiest to maintain. But I'm glad I'm not the only one who's not afraid to break an if onto multiple lines. :-D

[supercat9]

PIEBALDconsult wrote:

And that technique is a horror unto itself -- a Weasel-Goto.

Yeah, and writing

if (condition)
{
do_something();
}
else
{
do_something_else();
}

is really using "weasel-gotos" to obscure the fact that you're really writing:

if (condition) goto TRUE_CASE;
do_something_else();
goto END_OF_IF;
TRUE_CASE:
do_something();
END_OF_IF:

The goto statement itself is not inherently evil, but gets a bad rap for a couple reasons: -1- There is nothing inherent in the GOTO which gives any clue about where to find its target, or what the significance of its target might be; having the target of a branch always be either the first statement of an indented block that is being executed or has just completed, or the first statement following such a block (and having the control-flow instruction clearly imply which target applies) makes it much easier to see where the program flow is going. -2- Programs which use gotos (they are unavoidable when coding in things like assembly language), but where the overall program structure is consistent with the style indicated in (1) are generally easy for both humans and computers to analyze. If a program can clearly be divided into nested blocks such that there are no GOTOs into any block from anywhere outside it, program-flow analysis will generally be pretty easy. One of the major problems, in the days before structured programming, was that there were many pieces of code that had to go somewhere, and there wasn't any perceived reason not to put them anywhere that happened to be convenient. For example, consider the code I listed above; notice that a conditional branch will be taken in the true case, and an unconditional branch will be taken in the false case. If one case occurred much more frequently than the other, the code could be rewritten:

if (rare_condition) goto TRUE_CASE;
handle_false_case(); /* Optional */
END_OF_IF:
/* Continue on with code. */
/* After an unconditional jump somewhere, insert: */
TRUE_CASE:
handle_true_case();
goto END_OF_IF;

That approach will result in two branches taken in the true case, and none taken in the false case. In some situations, I've written code like that (when writing assembly code, and when performance was critical) but it's nasty to work with. Such code was the norm prior to the development of structured programming, and was the basis for much of the hatred related to GOTO. Incident

[PIEBALDconsult]

supercat9 wrote:

prior to the development of structured programming

Agreed. When I first learned programming it was in BASIC, which didn't have the wealth of control statements that many other languages have. Since learning Pascal and then C I haven't written a goto (other than within a switch statement).

supercat9 wrote:

a structure equivalent to "do{}while(0);"

Borland C/C++, in C++ mode will handle this:

# define once for ( int i##__line__ = 1 ; i##__line__ ; i##__line__-- )

once
{
printf ( "Howdy" ) ;

if ( argc < 3 )
{
    break ;
}

printf ( "Howdy" ) ;

}

Now I'll boot up one of my OpenVMS systems and try it there. Later: %CC-I-DECLINFOR, Placing a declaration in a for loop is a new feature in the C99 standard. Other C compilers may not support this extension. Later yet: I tried it with gcc version 3.2 (mingw special 20020817-1), which I use to pre-process C#, and it wouldn't work. It seems the others will replace __line__ (or __LINE__) before concatenating, but gcc wants to concatenate first and the replacement doesn't happen at all. :mad: I guess I need to find a new C pre-processor. :(

modified on Monday, December 15, 2008 9:52 PM

[Rob Grainger]

Personally, I have no real problem with returns in the middle of a function as long as there are no cleanup considerations, and as long as the procedure remains clear and readable. For cleanup issues, I believe C++ leads the way through RAII (Resource Acquisition Is Initialisation - RAII (Wikipedia), allowing code to be written in whichever manner makes it clearest to read - any resources are then guaranteed to be cleaned up on exit from the routine.

[supercat9]

The for-loop version is just plain nasty. It wastes a variable and a lot of code. Doing #define once while(0) and then

do {
some_code;
} once;

would avoid the wasted code. If one wanted to tag both ends of the loop to make clear what was going on, instead of just the tail, one could define some other word as a synonym for "do". Personally, though, I don't like using macros to alter the syntax of a language doing so will make practical something that would otherwise be impractical. To people familiar with do{}while(0); as a means of doing something once, that form will be as readable as anything else. For those unfamiliar with it, the macro would just add confusion. That it would be desirable for a language to include a feature to avoid silly always-true or always-false conditionals does not imply that it is desirable to kludge such features. BTW, if you really want your syntax, and if your code doesn't have to be thread-safe, you can limit the total number of extra variables program-wide to one:

#define once for(once_var=2;once_var>>=1; )

The global variable once_var will always be zero or one (doesn't matter which) except between the initial assignment and the initial test, so nested loops and recursion shouldn't pose problems. Still a horrible waste of code, but it should achieve the desired result.

[PIEBALDconsult]

supercat9 wrote:

The global variable once_var

Where do you declare it?

supercat9 wrote:

do{}while(0);

I've only seen that in the GCC documentation where it talks about "swallowing the semi-colon". I've certainly never seen it in production code, and I hope I never do. As others have also said, I don't use do/while, only while. And I don't write a loop that isn't a loop. Wait a minute... doesn't the 0 in while(0) have to exist somewhere as well?

[supercat9]

PIEBALDconsult wrote:

Where do you declare it?

I usually have a module with any global variables that are required by my macros. Not wonderful, but I don't know any better approach in C.

PIEBALDconsult wrote:

I've only seen that in the GCC documentation where it talks about "swallowing the semi-colon". I've certainly never seen it in production code, and I hope I never do.

When trying to write a macro that behaves like a void function but contains an 'if' statement, something like:

#define foo(x) do {if (x > 0) do_this();} while(0)

is clearer than:

#define foo(x) if (x > 0) do_this(); else

while nonetheless avoiding the difficulties associated with:

#define foo(x) {if (x > 0) do_this();}

or worse, the bug-prone

#define foo(x) if (x > 0) do_this()

which makes me cringe.

PIEBALDconsult wrote:

And I don't write a loop that isn't a loop.

What would be your preferred way of allowing a procedure to be done in multiple steps and abort at any step, while ensuring that any necessary cleanup would be completed? If you prefer a "goto" label to a do/while(0) structure with break statements, fine. That's actually my usual preference. Can you suggest anything better than the do/while(0) for people who dislike goto?

[PIEBALDconsult]

I don't mind it hidden in a macro; I expect to find odd things there, but I don't want to see it hung on the wall for all to see. Anyway, the original post's logic doesn't require anything like that.

[coolM]

In case of complex preconditions separate those to another function. This prevents structured-ifs and keeps the calling-function readable. If the conditions change (in thight iterative development this always happens), adaption gets easy.

bool ShouldDoSomething()
{
//no returns in middle of method, only on start
// or everywhere.. ;)
if(!flagA) return false;
if(!flagB) return false;
if(!flagC) return false;
if(!PromptUser() ) return false;
return true;
}

void callingFunction()
{
if( ShouldDoSomething() )
{
DoSomething();
}
else
{
DoOtherThing();
}
}

[PIEBALDconsult]

Then why not:

bool ShouldDoSomething()
{
return ( flagA && flagB && flagC && PromptUser() ) ;
}

which is much easier to read? :-D

[coolM]

you're right... got a bit infected by the horror around.

[notmasteryet]

Be aware of Junioria Developerus animal. Those species may decide that PromptUser() && flagA && flagB && flagC looks prettier. :) Cyclomatic complexity for && and if is the same. Some people believe that cyclomatic complexity number is directly related to code maintenance. And in this thread we did not really lower the number. Is the original code fine as is?

[PIEBALDconsult]

notmasteryet wrote:

Junioria Developerus

I suppose that would be a reason not to distance the tests from the code that uses it. And not to use a lot of negation.

[johannesnestler]

You are depending on the order of evaluation of your if-statement for your logic to work. Will the PromptUser() operand be evaluated before the entire set of && operations are performed? I think C++ has an "undefined order of evaluation". Who knows. It should be irrelevant. Writing code that depends on order of evaluation is bad programming practice, regardless of language. Sorry, but not knowing how your programming language works is bad practice (operator priority is very important in every language!). And depending on the order of your evaluations is the usual case in real world programming-logic. (I think you are mixing up here with the | and & operators and something a teacher told you once...). I think C++ has an "undefined order of evaluation". Who knows No - and I!!! ;P

[Megidolaon]

Ever thought about using &&?