Anonymized code from a method I'm working on right now.  Why do people continue to write code obtusely like this?

if (isUserInDb(user)){
    if (isUserAccountInOrder(user){
        if (isUserAccountNegativeBalance(user) {
            return status.Negative;
        return status.Positive;
        }
   return status.Invalid;
   }
return status.MissingAccount;

instead of the way I'd write it:

if (isUserInDb(user) == false) return status.MissingAccount;
if (isUserAccountInOrder(user) == false) return status.Invalid;
if (isUserAccountNegativeBalance(user)) return status.Negative;
return status.Positive;

So much easier to read. 

 

Apart from the truly horrible boolean == false, your version is a lot better.

Without going too far down the path; I prefer (if x() == false) rather than if (!x()) -- to me, it's too hard to miss that ! when I'm reading the code.

Of course, I'd really like to rename the method/return value:  instead of  if (x.isInvalid() == false)  I'd write  if (x.isValid())  but sometimes I don't get to change the API.

The reason why is because someone, somewhere, must have said to never have an if/else statement without the braces, and whoever wrote this code took it to heart that you MUST always have braces even if it hurts readability.

DrPepper:

Without going too far down the path; I prefer (if x() == false) rather than if (!x()) -- to me, it's too hard to miss that ! when I'm reading the code.

It never occurred to me that people might actually overlook that. I think my brain is so hard-wired from many years of C programming, that every non alphabetical character following a parenthesis is suspect.

On the other hand, recently I used "true" and "false" in an awk script, which in awk are just undefined variables, so both false.

ObiWayneKenobi:

The reason why is because someone, somewhere, must have said to never have an if/else statement without the braces, and whoever wrote this code took it to heart that you MUST always have braces even if it hurts readability.

 

 

And you MUST always nest them even if that hurts readability, too.  $DEITY forbid we get code like

 

if (isUserInDb(user) == false) { return status.MissingAccount; }
if (isUserAccountInOrder(user) == false) { return status.Invalid; }
if (isUserAccountNegativeBalance(user)) { return status.Negative; }
return status.Positive;

 

DrPepper:

Anonymized code from a method I'm working on right now.  Why do people continue to write code obtusely like this?

if (isUserInDb(user)){
    if (isUserAccountInOrder(user){
        if (isUserAccountNegativeBalance(user){
            return status.Negative;
        return status.Positive;
        }
    return status.Invalid;
    }
return status.MissingAccount;

 

Woah, you're right! That code uses multiple returns! Now that's hard to follow! Better add a return_status variable for clarity.

 This problem clearly calls for more metaprogramming.

#define check(op,f,s) if (op f(user)) return status.s;

check( !, isUserInDb                  , MissingAccount )
check( !, isUserAccountInOrder        , Invalid        )
check(  , isUserAccountNegativeBalance, Negative       )
return status.Positive;

#undef check

I interned with a major search engine at its Mountain View headquarters, and we had this rule for all Java code. They didn't even allow one-line curly pairs (except for array literals). I would have had to work around it with a cascade through ternary operators.

DrPepper:

Anonymized code from a method I'm working on right now.  Why do people continue to write code obtusely like this?

if (isUserInDb(user)){
    if (isUserAccountInOrder(user){
        if (isUserAccountNegativeBalance(user) {
            return status.Negative;
        return status.Positive;
        }
   return status.Invalid;
   }
return status.MissingAccount;

Yeah, that sucks. It should clearly be written as

return
    isUserInDb(user)?
        isUserAccountInOrder(user)?
            isUserAccountNegativeBalance(user)?
                status.Negative:
                status.Positive:
            status.Invalid:
        status.MissingAccount;

No bug due to unbalanced braces. No early returns. What's not to like?

What I think when I see "foo == false"? Guy doesn't get booleans. Guy needs to stay away from my upstream code.

emurphy:

And you MUST always nest them even if that hurts readability, too.  $DEITY forbid we get code like

 

if (isUserInDb(user) == false) { return status.MissingAccount; }
if (isUserAccountInOrder(user) == false) { return status.Invalid; }
if (isUserAccountNegativeBalance(user)) { return status.Negative; }
return status.Positive;

 

Your corrected version would be fine if every if save the first were changed to elseif.

 

da Doctah:

Your corrected version would be fine if every if save the first were changed to elseif.

Since each then branch contains a return without further condition, else wouldn't make a difference.

realmerlyn:

What I think when I see "foo == false"? Guy doesn't get booleans. Guy needs to stay away from my upstream code.

 

Good job ignoring his very reasonable explanation. A very astute and original insight!

DrPepper:

if (isUserAccountNegativeBalance(user) {
            return status.Negative;
        return status.Positive;
        }

This does not do what the author thinks it does. The return status.Positive will never be reached because it's already returned Negative. The second return should be outside the braces belonging to this if.

ASheridan:

The return status.Positive will never be reached

There's a brace mismatch there.

dhromed:

ASheridan:

The return status.Positive will never be reached

There's a brace mismatch there.

 

And the second and third lines have missing close-parentheses. I trust that is just careless anonymization, though, since it presumably compiles.

Or maybe DrPepper did it on purpose to make us all mentally trip over the invalid open brace before the if condition is closed?

 

DrPepper:

Without going too far down the path; I prefer (if x() == false) rather than if (!x()) -- to me, it's too hard to miss that ! when I'm reading the code.

Of course, I'd really like to rename the method/return value:  instead of  if (x.isInvalid() == false)  I'd write  if (x.isValid())  but sometimes I don't get to change the API.

 

This is interesting.  Yes, it's easy to miss a lonely exclamation mark.  Maybe what we should be doing is making it more obvious:

        if (!!!x())

or

       if (!!!!!x())

 

Then, as long as you have an odd number of exclamation marks, you probably won't miss it

 

dhromed:

realmerlyn:

What I think when I see "foo == false"?

It'd better be foo != true

aihtdikh:

And the second and third lines have missing close-parentheses.

Yep; comes from typing into the message box, rather than cut/paste real code.  Picky, Picky. 

realmerlyn:

What I think when I see "foo == false"? Guy doesn't get booleans

 

I use $foo === false a bit. Yes I work with PHP. No I'm not insane.

Zemm:

realmerlyn:

What I think when I see "foo == false"? Guy doesn't get booleans

 

I use $foo === false a bit. Yes I work with PHP. No I'm not insane.

strpos: Hey, let's make a function that can return 0 or false for completely different results but make it in a language where 0 == false

Ben L.:

Zemm:

realmerlyn:

What I think when I see "foo == false"? Guy doesn't get booleans

 

I use $foo === false a bit. Yes I work with PHP. No I'm not insane.

strpos: Hey, let's make a function that can return 0 or false for completely different results but make it in a language where 0 == false

I share(d) your pain.

On a completely unrelated note, I like Zemm's new avatar. A bird? Yeah!

 

Zecc:

Ben L.:

Zemm:

realmerlyn:

What I think when I see "foo == false"?Guy doesn't get booleans

 

I use $foo === false a bit. Yes I work with PHP. No I'm not insane.

strpos: Hey, let's make a function that can return 0 or false for completely different results but make it in a language where 0 == false

I share(d) your pain.

Well I'm starting a new job tomorrow. Apparently there's some Perl involved, so I'm not yet sure if that's better or worse than PHP. I guess I'll find out in less than 12 hours.

Zecc:

On a completely unrelated note, I like Zemm's new avatar. A bird? Yeah!

Did the sig make it too obvious? :P Was tossing up between that one and "What does that mean?"

Zemm:

Zecc:

On a completely unrelated note, I like Zemm's new avatar. A bird? Yeah!

Did the sig make it too obvious? :P Was tossing up between that one and "What does that mean?"

To be honest, I only noticed the signature afterwards. I just recognised the bird for some reason, then noticed the kid which confirmed my suspicion.

Zecc:

I just recognised the bird for some reason, then noticed the kid which confirmed my suspicion.

Do you have the entire album? It did sell over a million copies. My non-single favourites are Etoh and Live at Dominoes. "Flight 22 is off to Honolulu"

No, I don't have their album. I just know them from Frontier Psychiatrist's videoclip, which I've seen a couple of times on TV, and from YouTube where I heard a couple of songs more. Namely, I remember Electricity and Run DNA. I also remember having saved the FLV for Frontier Psychiatrist somewhere.

I would personally probably do something evil like

<type> status_codes[ = {status.MissingAccount , status.Positive, status.MissingAccount, ... };

return status_codes[ (isUserInDb(user)) | (isUserAccountInOrder(user)<<1) | (isUserAccountNegativeBalance<<2) ];

No branching at all!  (And only slightly dangerous by assuming the isUserX() functions return 0 or 1).

too_many_usernames:

(isUserAccountNegativeBalance<<2) ];

I'm deeply hoping that this isn't a typo and you really would OR a function pointer in with the other two bits.

Ben L.:

too_many_usernames:

(isUserAccountNegativeBalance<<2) ];

I'm deeply hoping that this isn't a typo and you really would OR a function pointer in with the other two bits.

It's an optimization trick.

too_many_usernames:

I would personally probably do something evil like

<type> status_codes[ = {status.MissingAccount , status.Positive, status.MissingAccount, ... };

return status_codes[ (isUserInDb(user)) | (isUserAccountInOrder(user)<<1) | (isUserAccountNegativeBalance<<2) ];

No branching at all!  (And only slightly dangerous by assuming the isUserX() functions return 0 or 1).

That's really horrible code; requiring an enum for each possible combination of bit values.  And, you expect the caller to be able to interpret the results (i.e., which enums mean "isUserInDB"? )

In C#, you could at the least return an anonymous object:

return new { isUserInDb= isUserInDb(user), isUserInAccountInOrder = isUserAccountInOrder(user), isUserAccountNegativeBalance = isUserAccountNegativeBalance(user) };

Then the caller can look at the object's members and see what might have happened

DrPepper:

too_many_usernames:

I would personally probably do something evil like

<type> status_codes[ = {status.MissingAccount , status.Positive, status.MissingAccount, ... };

return status_codes[ (isUserInDb(user)) | (isUserAccountInOrder(user)<<1) | (isUserAccountNegativeBalance<<2) ];

No branching at all!  (And only slightly dangerous by assuming the isUserX() functions return 0 or 1).

That's really horrible code; requiring an enum for each possible combination of bit values.  And, you expect the caller to be able to interpret the results (i.e., which enums mean "isUserInDB"? )

In C#, you could at the least return an anonymous object:

return new { isUserInDb= isUserInDb(user), isUserInAccountInOrder = isUserAccountInOrder(user), isUserAccountNegativeBalance = isUserAccountNegativeBalance(user) };

Then the caller can look at the object's members and see what might have happened

 

Or you could just use the enum as a flags object and declare it properly.

 

DrPepper:

too_many_usernames:

I would personally probably do something evil like

<type> status_codes[ = {status.MissingAccount , status.Positive, status.MissingAccount, ... };

return status_codes[ (isUserInDb(user)) | (isUserAccountInOrder(user)<<1) | (isUserAccountNegativeBalance<<2) ];

No branching at all!  (And only slightly dangerous by assuming the isUserX() functions return 0 or 1).

That's really horrible code; requiring an enum for each possible combination of bit values.

Bollocks. Doesn't need a separate enum for each combination of bit values, merely the correct enum values in the status_codes[] initializer:

status_code_t status_codes[] = {
    status.MissingAccount, /* --- */
    status.Invalid,        /* t-- */
    status.MissingAccount, /* -t- */
    status.Positive,       /* tt- */
    status.MissingAccount, /* --t */
    status.Invalid,        /* t-t */
    status.MissingAccount, /* -tt */
    status.Negative        /* ttt */
};

Inefficient though, as it can't do short-circuit evaluation of the Boolean expression; this may also make it incorrect, depending on what UserAccountNegativeBalance() does when isUserAccountInOrder() returns 0, and what isUserAccountInOrder() does when isUserInDb() returns 0.

The nested ternary conditional operator solution does not suffer from this problem.

flabdablet:

The nested ternary conditional operator solution does not suffer from this problem.

But I want it to suffer.

 C++ allows if (not ) which is really hard to miss. I've started using it and it drives one of our programmers wild "because it's different - people won't know what it does" ....

#define if(x) if(!x)

joe.edwards:

#define if(x) if(!x)

The WTFs that can come from improper or nefarious use of the C preprocesser deserver their own sidebar. Best not go down into that hole.

thosrtanner:

 C++ allows if (not ) which is really hard to miss.

It really isn't hard to miss.

PJH:

thosrtanner:

 C++ allows if (not ) which is really hard to miss.

It really isn't hard to miss.

I missed the fact it was a keyword,but I wouldn't miss it if it wasn't.