Can't see the forest for the trees



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

    <font size="2" face="Consolas"><font size="2" face="Consolas"></font></font> 



  • 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 said:

    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 said:

    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 said:

    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 said:

    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 said:

    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 said:

    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 said:

    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 said:

    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 said:

    The return status.Positive will never be reached

    There's a brace mismatch there.



  • @dhromed said:

    @ASheridan said:

    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 said:

    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 said:

    @realmerlyn said:

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

    It'd better be

    [code]foo != true [/code]



  • @aihtdikh said:

    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 said:

    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 said:

    @realmerlyn said:

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

    @Zemm said:

    @realmerlyn said:

    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 said:

    @Ben L. said:

    @Zemm said:

    @realmerlyn said:

    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 said:

    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 said:

    @Zecc said:
    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 said:

    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 said:

    (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. said:

    @too_many_usernames said:
    (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 said:

    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 said:

    @too_many_usernames said:

    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 said:

    @too_many_usernames said:

    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.


  • Considered Harmful

    @flabdablet said:

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


  • Considered Harmful

    #define if(x) if(!x)



  • @joe.edwards said:

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


  • Discourse touched me in a no-no place

    @thosrtanner said:

     C++ allows if (not ) which is really hard to miss.
    It really isn't hard to miss.



  • @PJH said:

    @thosrtanner said:
     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.


Log in to reply