Gotta make sure it's not invalid

Snuffles

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 not return result == Second;?

Response:
"That's more complicated! How about..."

if (result == Invalid)
    return false;
return result == Second;

Facepalm.

chubertdev

Future-proofing! What if Second becomes 1, and First becomes 0?

Maciejasjmj

@chubertdev said:

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

Snuffles

@Maciejasjmj said:

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

FrostCat

@Snuffles said:

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

accalia

I know! the forgot the most important ENUM value! FILE_NOT_FOUND

Snuffles

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 check foo's body to see its return values was upsetting.

Snuffles

@FrostCat said:

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.

FrostCat

@Snuffles said:

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.

EvanED

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.

Planar

    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.

FrostCat

@EvanED said:

Doesn't mean it's not dumb.

Oh, no doubt.

flabdablet

@Snuffles said:

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

chubertdev

@flabdablet said:

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

Evo

@chubertdev said:

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.

chubertdev

Yeah, because that requires two brain cells.

dkf

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.

another_sam

@dkf said:

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.

chubertdev

Reminds me of the people that declare every variable they could possibly reference at the beginning of a long method.

HardwareGeek

That is, at least sometimes, the fault of the language, not the programmer.

chubertdev

That would be the exception to the rule.

Snuffles

@another_sam said:

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

CreatedToDislikeThis

Might've been more fair if you didn't delete the baz and biz cases from the multiple-return version of the code?

MathNerdCNU

All spaghetti falls to the ground when I drop it.

Am I goto-ing right?

another_sam

@Snuffles said:

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.

MathNerdCNU

https://www.youtube.com/watch?v=DhrZxSoLmgA

Snuffles

@another_sam said:

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.

Snuffles

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

CreatedToDislikeThis

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!

another_sam

@Snuffles said:

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.

Snuffles

@another_sam said:

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.

another_sam

@Snuffles said:

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.

chubertdev

@another_sam said:

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.

Evo

@dkf said:

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.

dkf

Early exit makes a lot of sense for a precondition check failure. Especially when the operation that is being guarded is fairly complicated.

Arantor

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?

chubertdev

if(!Page.IsValid) { return; }

VinDuv

I work on C code with the following rules:

No early returns
No gotos
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, only uint8_t and friends
Hungarian notation for every variable, function, struct…
EDIT: No ternary operator

Guess what: It doesn’t magically prevent bugs.

aliceif

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

VinDuv

@aliceif said:

Yet they don't ban the ? : operator?

Ah yes, I missed that one. Adding it…

dkf

@VinDuv said:

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…)

chubertdev

Developers must sit on their hands at all times.