Avoid return statement in the middle - horror or not?

Georgi Atanasov

I have a coding rule - avoid "return" somewhere in the middle of a method, try using local variables to compensate instead. Observing this leads to the following (seen in existing code):

if(flagA)
{
if(flagB)
{
if(flagC)
{
if(PromtUser())
{
DoSomething();
}
else
{
DoOtherThing();
}
}
else
{
DoOtherThing();
}
}
else
{
DoOtherThing();
}
}
else
{
DoOtherThing();
}

How I would have written this is:

if(flagA)
{
if(flagB)
{
if(flagC)
{
if(PromptUser())
{
return;
}
}
}
}

DoOtherThing();

I was wondering how you guys feel about it?

Thanks, Georgi

Robert C Cartaino

You forgot to execute DoSomething() in your second example. That aside... I don't necessarily dislike "return in the middle." But I expect early returns to be an error condition (i.e. invalid parameter, resource not obtained, etc) to keep the method from continuing. In your example, the early return seems to be the normal, successful condition and that makes me uncomfortable. So looking at your code samples... Definitely not the first example. If you ever want to add more logic around DoOtherThing(), you've essentially "cut-and-pasted" your code all over the place (a big no-no). There are a lot of conditions where DoOtherThing() would be executed. According to your logic, if each process (A, B, and C) passes, ask the user if they want to DoSomething(). If user says "no" or any of the processes fail, then DoOtherThing(). To make my intentions clear, I would try and write the logic exactly as I would describe it (of course, the variables would be more "English-Like" if I knew their purpose). Maybe something like this:

// assume for the moment, we don't need to DoSomething()
bool DoSomethingNeeded = false;

if (FlagA && FlagB && FlagC)
{
// if all processes passed, ask the user if we need to DoSomething()
DoSomthingNeeded = PromptUser();
}

if (DoSomethingNeeded)
{
DoSomething();
}
else
{
DoOtherThing();
}
return;

PIEBALDconsult

if ( flagA && flagB && flagC && PromptUser() )
{
DoSomething();
}
else
{
DoOtherThing();
}

return ;

Keep in mind that the && operator is short-circuit.

ehuysamer

The second certainly looks cleaner; but sometimes you need to do cleanup (dispose objects, free memory, release handles, etc.) before exiting a function / method. If you add code later that needs cleanup, and already coded the second way, you'll have a lot to change... in addition, code that is already out there running stable is more valuable than new code, since you risk introducing bugs by changing from your second example back to the first. So I'd definitely go for the first - good habits breed consistency.

Tristan Rhodes

I've got to say, i don't agree with that rule. I will aim to return out the function either very early, or at the end. Trivial conditions should be treated as such, and I'd rather they returned at the top, than add a unnecessary level of nested bracers. I have seen the result of blindly following that practice to it's conclusion - a mountain of if / else statements with embedded switches that was 12 deep in places. I would rather see: if (guardCondition1) return guard1Default; if (guardCondition2) return guard2Default; if (guardCondition3) return guard3Default; //Do 20 lines of Complex Logic Here return result; That said, throwing return statements in without any real eye on program flow can cause just as many problems. Regards Tris

------------------------------- Carrier Bags - 21st Century Tumbleweed.

cokkiy

Maybe WF fit such situation

The God created the world. The programer made the world easy.

Robert C Cartaino

[Message Deleted]

Robert C Cartaino

I completely agree with your sentiment. But in this specific example, it looks like processes generating FlagA, FlagB, and FlagC should always execute, regardless of whether any of the previous processes fail. Your guardConditionals don't allow for that.

Lost User

According those many sites, C and C++ do use short-circuiting. As a well-defined part of the language standard. Which means that it's fine to reply on, as long as you feel it won't impact readability. Java also seems to behave differently from what you said: http://www.student.cs.uwaterloo.ca/~cs132/Weekly/W02/SCBooleans.html[^] And anyway, it's very pointless to put extra boolean operators (&&, ||) in a language without defining them to be short-circuiting. There are | and & already, and if there is no short-circuiting they'd be the same.

Graham Bradshaw

Robert.C.Cartaino wrote:

I think C++ has an "undefined order of evaluation".

C and C++ are both strict left to right evaluation order for logical operators, and always use short-circuiting. It allows constructs such as

void* p = GetPointer(); // this may fail, and return NULL

if (p != NULL && p->SomeMethod()
....

p->SomeMethod() will only be called if p is not NULL

riced

Having been brought up on 'Structured Programming' I used to think that one-entry and one-exit point was the only way to write procedures. Now I'm in favour of your approach - i.e. use guard conditions and exit early if need be. Oh and keep the procedure small, roughly no more than can fit on an A4 page. One example of how not to do it that I came across was a C++ function that had around 500 lines of code with about 15 returns scattered throughout. It also had a couple of places where it could throw an exception! David Rice

PIEBALDconsult

see this[^] " && Operator (C# Reference) The conditional-AND operator (&& ) performs a logical-AND of its bool operands, but only evaluates its second operand if necessary. " " x && y if x is false, y is not evaluated (because the result of the AND operation is false no matter what the value of y may be). This is known as "short-circuit" evaluation. " Any language that doesn't do it that way is not a "real" programming language.

Robert C Cartaino

harold aptroot wrote:

Java also seems to behave differently from what you said:

I think you are right. I was looking at this section[^] of the Java language specification: 15.7.2 Evaluate Operands before Operation The Java programming language also guarantees that every operand of an operator (except the conditional operators &&, ||, and ? : ) appears to be fully evaluated before any part of the operation itself is performed. They do explicitly exempt the logical operators. Let's hope that && is not overloaded -- in C++ at least -- then short circuiting does not apply. Then I was reading this on C++ Sequence Points[^]. It says that "the left operand of the logical AND operator is completely evaluated and all side effects completed before continuing. There is no guarantee that the right operand of the logical AND operator will be evaluated. Of course, it doesn't say that is guaranteed not to be evaluated, either. Then it says this: "The controlling expression in a selection (if or switch) statement. The expression is completely evaluated and all side effects completed before the code dependent on the selection is executed." So, who knows. My conclusion would be that a C/C++ compiler is indeed supposed to do left-right evaluation and short circuiting. But I still say it's irrelevant. If I have to hunt that hard to get down into the bowels of how a compiler works to determine how my code will execute, I say it's a bad programming practice. I'll take these guys word for it: ...The moral is that writing code that depends on order of evaluation is a bad programming practice in any language. Naturally, it is necessary to know what things to avoid, but if you don't know how they are done on various machines, you won't be tempted to take advantage of a particular implementation. -- The C Programming Language[

Lost User

Ah good ol' C and C++ with their lack of guarantees :) Now that is a real horror :laugh:

PIEBALDconsult

From the ANSI C99 spec I have: " 6.5.13-4 Unlike the bitwise & operator, the && operator guarantees left-to-right evaluation; there is a sequence point after the evaluation of the first operand. If the first operand compares equal to 0, the second operand is not evaluated. " Even Ritchie's C manual from 1974 says it's left-to-right. (It appears that B does not have these operators.) In the K&R book, they're talking about a[i]=i++, not the && operator, plus that's a very old book. You may certainly go right ahead and write code any way you like... I do.

PIEBALDconsult

Having one return statement is still a worthwhile goal. You just have to look at it both ways and decide which is "better" in each individual case. I don't recall encountering any situations that required multiple return statements.

Michael Dunn

You can't make a rule like that because there is no one correct rule that works for every single function you're going to write in your life. Well, you can make the rule, but doing so will be counter-productive. Why create a state machine with local variables in a function (and don't forget to test every possible state!), just to avoid using a perfectly valid language feature?

--Mike-- Visual C++ MVP :cool: LINKS~! CP SearchBar v3.0 | C++ Forum FAQ I work for Keyser Söze

Andrew Torrance

Single exit , single entry . Its a mantra that leads to clarity how about if( !FlagA || !FlagB || !FlagC) // Select Variant on !Flagx or Flagx == false depending on language DoOtherThing(); else { if(PromptUser()) DoSomething(); } Not a horror though. My use of single entry and exit points in functions leads to more debates than anything. (My how the winter nights fly by)

Robert C Cartaino

Andrew Torrance wrote:

if( !FlagA || !FlagB || !FlagC) // Select Variant on !Flagx or Flagx == false depending on language DoOtherThing(); else { if(PromptUser()) DoSomething(); }

...except if PromptUser() fails, you also have to execute DoOtherThing(). So you need to add to your code:

if( !FlagA || !FlagB || !FlagC) // Select Variant on !Flagx or Flagx == false depending on language
DoOtherThing();
else
{
if(PromptUser())
DoSomething();
else DoOtherThing();
}

PIEBALDconsult

How is that better than what I posted?