Avoid return statement in the middle - horror or not?
-
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)
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 executeDoOtherThing(). 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();
} -
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)
How is that better than what I posted?
-
[Message Deleted]
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.
-
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.
Indeed. And should we question the order/precedence of the other operators as well? Can we trust
3 + 4 * 5to 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. -
How is that better than what I posted?
-
How is that better than what I posted?
-
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
it all depends on how much coffe i've had for the day...
do or do not, there is no try
-
it all depends on how much coffe i've had for the day...
do or do not, there is no try
Drinking coffee leads to good code. Drinking "energy drinks" leads to poor code.
-
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
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
-
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