Avoid return statement in the middle - horror or not?
-
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
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(); }
-
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(); }
That's simply the first snippet inverted and made even uglier. It's still a maintenance nightmare.
-
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
Where's the call to DoSomething? I'm pretty sure that's not applicable to the original post.
-
Where's the call to DoSomething? I'm pretty sure that's not applicable to the original post.
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
-
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
Still looks like a potentially infinite loop.
-
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
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
-
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
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.
-
Still looks like a potentially infinite loop.
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
-
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
: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|
-
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
::Shudder::
-
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 aroundDoOtherThing(), you've essentially "cut-and-pasted" your code all over the place (a big no-no). There are a lot of conditions whereDoOtherThing()would be executed. According to your logic, if each process (A, B, and C) passes, ask the user if they want toDoSomething(). If user says "no" or any of the processes fail, thenDoOtherThing(). 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;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
-
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
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.
-
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
While having many
returnstatements might be questionable, I think readability is much more important. So I would always allow for multiplereturnstatements 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 singlereturn. 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. -
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
Nah,
gotois 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. -
Nah,
gotois 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.When you do need a
goto(and I haven't since I quit BASIC) it's best to just use it. C# -- forswitchstatements. -
While having many
returnstatements might be questionable, I think readability is much more important. So I would always allow for multiplereturnstatements 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 singlereturn. 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 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? -
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
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.
-
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.
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
ifonto multiple lines. :-D -
: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 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 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
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