Gotta make sure it's not invalid
-
struct Bar
{
enum { Invalid = -1, First = 1, Second = 0 };int foo(); // returns an enum value as an int bool frobulate() { int result = foo(); return (result != Invalid && !result == true); } };
Code review:
"Why notreturn result == Second;
?Response:
"That's more complicated! How about..."if (result == Invalid) return false; return result == Second;
Facepalm.
-
Future-proofing! What if Second becomes 1, and First becomes 0?
-
Future-proofing! What if Second becomes 1, and First becomes 0?
With how the enum values are laid out, I wouldn't be surprised if that was actually how it used to be...
-
With how the enum values are laid out, I wouldn't be surprised if that was actually how it used to be...
Nope, the enum was that way from the start. It's so that Second < First (yes really).
-
[code]enum { Invalid = -1, First = 1, Second = 0 };[/code]
That is the kind of bullshit up with which I will not put. Kindly slap that person for the rest of us.
-
I know! the forgot the most important ENUM value!
FILE_NOT_FOUND
-
I'm surprised no one has commented on the fact that
foo()
returns an enum value as an int and then compares it to other enums. Coder was mystified when I explained why requiring me to checkfoo
's body to see its return values was upsetting.
-
That is the kind of bullshit up with which I will not put. Kindly slap that person for the rest of us.
I really hate it when stuff like this comes up in reviews. Especially with the passive-aggressive eye-roller who thinks anything that doesn't cause incorrect behavior right now is not up for discussion.
-
I'm surprised no one has commented on the fact that foo() returns an enum value as an int and then compares it to other enums.
According to http://stackoverflow.com/questions/11314545/casting-enum-definition-to-unsigned-int (C99 6.4.4.3), enums are ints (or maybe unsigned int, or in earlier versions, "of a type compatible with int), so that's probably why nobody noted it.
-
Doesn't mean it's not dumb. Why have the enum in the first place if you're not going to bother to use it?
If that code came in for my review, and the int return couldn't be changed, I'd at least ask for an
assert(result == Invalid || result== First || result == Second)
after it before I'd be comfortable signing off.
-
if (result == Invalid) return false; return result == Second;
That's good. Now you can optimize it to
if (Second == Invalid) return false; return result == Second;
and let the compiler remove the useless test.
-
-
"That's more complicated! How about..."
if (result == Invalid)
return false;
return result == Second;Breaks single point of return rule. Should be
return result == Invalid? false: result == Second? true: false;
Remember, kids, the only trustworthy boolean is a literal boolean.
-
Breaks single point of return anti-pattern. Should be <code>return result == Invalid? false: result == Second? true: false;</code>
Remember, kids, the only trustworthy boolean is a literal boolean.
FTFY
-
FTFY
I don't understand why you'd call single point of return an anti-pattern. Everybody knows you should do it like this:
bool func() { int result = foo(); bool ret; if(result != Invalid) { ret = result != Invalid; goto funcDone; } ret = result == second; goto funcDone; funcDone: return ret; }
See? A clear, single point of return. Never have to search for the return statement.
-
Yeah, because that requires two brain cells.
-
You are aware that programmers that promote the single-point-of-return anti-pattern also tend to go straight for the nothing-that-might-ever-look-like-
goto
-ever anti-pattern? The combination is where things start to get stinky, as they try to combine all their control flow logic with their data logic through loads of itty-bitty little flags.
-
You are aware that programmers that promote the single-point-of-return anti-pattern also tend to go straight for the nothing-that-might-ever-look-like-goto-ever anti-pattern?
You're damn right, because we like the code to start at the top and go to the bottom. Take your spaghetti elsewhere.
-
Reminds me of the people that declare every variable they could possibly reference at the beginning of a long method.
-
That is, at least sometimes, the fault of the language, not the programmer.
-
That would be the exception to the rule.
-
You're damn right, because we like the code to start at the top and go to the bottom. Take your spaghetti elsewhere.
bool res = false; if (foo) { // things if (bar) { if (baz) { //stuff } else if (biz) { //other } } } return res;
vs.
if (!foo) return false; //things if (!bar) return false; ....
case closed
-
Might've been more fair if you didn't delete the baz and biz cases from the multiple-return version of the code?
-
All spaghetti falls to the ground when I drop it.
Am I goto-ing right?
-
case closed
Your example is contrived, your "better" version misses some conditions, refactoring is harder when the "else" clauses become required, and you really need to learn to decompose your functions.
In short, you're a bad programmer and you should feel bad.
-
-
Your example is contrived, your "better" version misses some conditions, refactoring is harder when the "else" clauses become required, and you really need to learn to decompose your functions.
In short, you're a bad programmer and you should feel bad.
I am a bad programmer and I do feel bad. However, I did almost this exact refactoring today, which was promptly reverted by the original programmer with the commit message "one exit point". He is a bad programmer as well, but in addition his ideas are bad too.
-
@created_just_to_disl said:
Might've been more fair if you didn't delete the baz and biz cases from the multiple-return version of the code?
That's what the ellipsis is for. That and an indication of my sloth.
-
Let's do another comparison
if (foo) { // things if (bar) ...... } return ......;
vs.
if (!foo) return false; //things if (!bar) return false; if (baz) { // stuff return true; } else { // other return false; }
Huh! The single-return version looks much better suddenly!
-
I am a bad programmer and I do feel bad
Okay, so now I'll let up on you and say that my post was a little tongue-in-cheek. There are cases in which the single exit point rule should be broken, your example just doesn't show it. Single exit point is desirable but should be considered in balance with all the other rules we follow when writing code. It's a guideline to help achieve readability, not a goal in itself, and not a cold hard rule. And as always, opinions differ on what's most readable.
-
Okay, so now I'll let up on you and say that my post was a little tongue-in-cheek.
I of course wasn't serious. I am a great programmer and I only feel bad that others have to live without understanding my greaty greatness.
All seriousness aside, the notion of "single exit point" in languages with exceptions is just an anachronism.
-
All seriousness aside, the notion of "single exit point" in languages with exceptions is just an anachronism.
But... exceptions are an exception to the rule.
-
It's a guideline to help achieve readability
I find single exit methods harder to read.
Not saying that it's better. It seems to be a personal preference.
-
You are aware that programmers that promote the single-point-of-return anti-pattern also tend to go straight for the nothing-that-might-ever-look-like-
goto
-ever anti-pattern? The combination is where things start to get stinky, as they try to combine all their control flow logic with their data logic through loads of itty-bitty little flags.
Really? I had no idea... And I don't do sarcasm.
-
Early exit makes a lot of sense for a precondition check failure. Especially when the operation that is being guarded is fairly complicated.
-
I'm always a fan of the notion that if you have a complex series of operations to test and one test early on means you don't have to bother with the rest, why not run away as soon as you know you're done?
-
if(!Page.IsValid) { return; }
-
I work on C code with the following rules:
- No early returns
- No
goto
s - No
continue
- At most one
break
in loops - Maximum block nesting of 5
- At most 6 parameters and 6 local variables per function
- Local variables must be declared at the start of the function
- No basic types like
int
, onlyuint8_t
and friends - Hungarian notation for every variable, function, struct…
- EDIT: No ternary operator
Guess what: It doesn’t magically prevent bugs.
-
EDIT: This was a reply to an old version of the above post.
Yet they don't ban the
? :
operator?
Yay for dumb rules.
Filed under: If you dumbify, go all the way
-
-
At most one break in loops
They allow one? What persuaded them to be so radical?
(Do they ban
longjmp()
? That would be a funny way to circumvent the rules while obeying the letter of them…)
-
Developers must sit on their hands at all times.