The Daily WTF: Curious Perversions in Information Technology
Welcome to TDWTF Forums Sign in | Join | Help
in Search

Can't see the forest for the trees

Last post 12-01-2012 4:25 PM by Zecc. 38 replies.
Page 1 of 1 (39 items)
Sort Posts: Previous Next
  • 11-21-2012 3:06 PM

    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. 

     

  • 11-21-2012 3:20 PM In reply to

    • TGV
    • Top 75 Contributor
    • Joined on 10-09-2005
    • Posts 703

    Re: Can't see the forest for the trees

    Apart from the truly horrible boolean == false, your version is a lot better.
  • 11-21-2012 3:32 PM In reply to

    Re: Can't see the forest for the trees

    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.

  • 11-21-2012 3:49 PM In reply to

    Re: Can't see the forest for the trees

    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.
    The Daily WTF Forums. You will never find a more wretched hive of scum and villainy.
  • 11-21-2012 4:10 PM In reply to

    • TGV
    • Top 75 Contributor
    • Joined on 10-09-2005
    • Posts 703

    Re: Can't see the forest for the trees

    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.

  • 11-21-2012 4:19 PM In reply to

    • emurphy
    • Top 100 Contributor
    • Joined on 01-14-2005
    • Granada Hills, CA
    • Posts 576

    Re: Can't see the forest for the trees

    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;

     

  • 11-21-2012 4:20 PM In reply to

    Re: Can't see the forest for the trees

    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.

  • 11-21-2012 6:44 PM In reply to

    Re: Can't see the forest for the trees

     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
    
  • 11-21-2012 7:48 PM In reply to

    Re: Can't see the forest for the trees

    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.
  • 11-21-2012 10:36 PM In reply to

    Re: Can't see the forest for the trees

    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?

  • 11-22-2012 12:00 AM In reply to

    Re: Can't see the forest for the trees

    What I think when I see "foo == false"? Guy doesn't get booleans. Guy needs to stay away from my upstream code.
  • 11-22-2012 2:51 AM In reply to

    Re: Can't see the forest for the trees

    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.

     

  • 11-22-2012 3:18 AM In reply to

    • Bulb
    • Top 500 Contributor
    • Joined on 07-29-2008
    • Prague, Czech Republic
    • Posts 220

    Re: Can't see the forest for the trees

    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.
  • 11-22-2012 5:44 AM In reply to

    Re: Can't see the forest for the trees

    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!


    In complex analysis, a meromorphic function on an open subset D of the complex plane is a function that is holomorphic on all D except a set of isolated points

    Filed under: ,
  • 11-22-2012 5:59 AM In reply to

    Re: Can't see the forest for the trees

    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.

  • 11-22-2012 6:50 AM In reply to

    Re: Can't see the forest for the trees

    ASheridan:
    The return status.Positive will never be reached

    There's a brace mismatch there.


    In complex analysis, a meromorphic function on an open subset D of the complex plane is a function that is holomorphic on all D except a set of isolated points

    Filed under:
  • 11-22-2012 8:00 AM In reply to

    Re: Can't see the forest for the trees

    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?

     

    Elicit this site's eponymous ejaculation.
  • 11-22-2012 12:45 PM In reply to

    Re: Can't see the forest for the trees

    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

     

  • 11-22-2012 2:02 PM In reply to

    • Franco
    • Not Ranked
    • Joined on 11-22-2012
    • Posts 1

    Re: Can't see the forest for the trees

    dhromed:

    realmerlyn:
    What I think when I see "foo == false"?
    It'd better be foo != true
  • 11-22-2012 2:39 PM In reply to

    Re: Can't see the forest for the trees

    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. 

  • 11-22-2012 5:50 PM In reply to

    • Zemm
    • Top 50 Contributor
    • Joined on 11-26-2007
    • Gold Coast, Australia
    • Posts 1,380

    Re: Can't see the forest for the trees

    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.

    Filed Under: Can you think of something that talks, other than a person?
  • 11-23-2012 12:23 AM In reply to

    • Ben L.
    • Top 10 Contributor
    • Joined on 12-22-2010
    • Inventor of the 186-hour work week
    • Posts 3,606

    Re: Can't see the forest for the trees

    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
  • Morbs is the smartest!
  • 11-25-2012 6:07 AM In reply to

    • Zecc
    • Top 25 Contributor
    • Joined on 06-12-2007
    • and hasn't left since.
    • Posts 2,057

    Re: Can't see the forest for the trees

    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!

     

    If mixed metaphors were illegal, I'd be having an indigestion.
    typeof NaN == 'number'
    var ò_ó, ಠ⁔ಠ, ᄒᆺᄒ, ᅙᅳᅙ, ᖛᨓᖜ, ꖴᅩꖴ, ఠᨋఠ; // Naming your variables is serious business
  • 11-25-2012 6:53 AM In reply to

    • Zemm
    • Top 50 Contributor
    • Joined on 11-26-2007
    • Gold Coast, Australia
    • Posts 1,380

    Re: Can't see the forest for the trees

    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?"
    Filed Under: Can you think of something that talks, other than a person?
  • 11-25-2012 7:00 AM In reply to

    • Zecc
    • Top 25 Contributor
    • Joined on 06-12-2007
    • and hasn't left since.
    • Posts 2,057

    Re: Can't see the forest for the trees

    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.

    If mixed metaphors were illegal, I'd be having an indigestion.
    typeof NaN == 'number'
    var ò_ó, ಠ⁔ಠ, ᄒᆺᄒ, ᅙᅳᅙ, ᖛᨓᖜ, ꖴᅩꖴ, ఠᨋఠ; // Naming your variables is serious business
  • 11-25-2012 7:08 AM In reply to

    • Zemm
    • Top 50 Contributor
    • Joined on 11-26-2007
    • Gold Coast, Australia
    • Posts 1,380

    Re: Can't see the forest for the trees

    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"
    Filed Under: Can you think of something that talks, other than a person?
  • 11-25-2012 7:36 AM In reply to

    • Zecc
    • Top 25 Contributor
    • Joined on 06-12-2007
    • and hasn't left since.
    • Posts 2,057

    Re: Can't see the forest for the trees

    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.
    If mixed metaphors were illegal, I'd be having an indigestion.
    typeof NaN == 'number'
    var ò_ó, ಠ⁔ಠ, ᄒᆺᄒ, ᅙᅳᅙ, ᖛᨓᖜ, ꖴᅩꖴ, ఠᨋఠ; // Naming your variables is serious business
  • 11-26-2012 1:20 PM In reply to

    Re: Can't see the forest for the trees

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

  • 11-27-2012 9:42 PM In reply to

    • Ben L.
    • Top 10 Contributor
    • Joined on 12-22-2010
    • Inventor of the 186-hour work week
    • Posts 3,606

    Re: Can't see the forest for the trees

    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.
  • Morbs is the smartest!
  • 11-28-2012 8:44 AM In reply to

    Re: Can't see the forest for the trees

    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.
  • 11-28-2012 4:20 PM In reply to

    Re: Can't see the forest for the trees

    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

  • 11-28-2012 4:30 PM In reply to

    Re: Can't see the forest for the trees

    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.

     

  • 11-29-2012 10:10 AM In reply to

    Re: Can't see the forest for the trees

    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.

  • 11-29-2012 10:23 AM In reply to

    Re: Can't see the forest for the trees

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

    But I want it to suffer.

    Signatures are stupid.
  • 11-30-2012 3:36 PM In reply to

    Re: Can't see the forest for the trees

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

  • 11-30-2012 4:31 PM In reply to

    Re: Can't see the forest for the trees

    #define if(x) if(!x)
    Signatures are stupid.
  • 11-30-2012 5:35 PM In reply to

    Re: Can't see the forest for the trees

    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.

  • 12-01-2012 1:47 PM In reply to

    • PJH
    • Top 10 Contributor
    • Joined on 02-14-2007
    • Newcastle, UK
    • Posts 3,906

    Re: Can't see the forest for the trees

    thosrtanner:
     C++ allows if (not ) which is really hard to miss.
    It really isn't hard to miss.
    "Because you watched 'The Very Hungry Caterpillar,' we recommend 'The Human Centipede.'"
    --
    UED - Countryside: To kill Piers Morgan
  • Parp!
  • 12-01-2012 4:25 PM In reply to

    • Zecc
    • Top 25 Contributor
    • Joined on 06-12-2007
    • and hasn't left since.
    • Posts 2,057

    Re: Can't see the forest for the trees

    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.

    If mixed metaphors were illegal, I'd be having an indigestion.
    typeof NaN == 'number'
    var ò_ó, ಠ⁔ಠ, ᄒᆺᄒ, ᅙᅳᅙ, ᖛᨓᖜ, ꖴᅩꖴ, ఠᨋఠ; // Naming your variables is serious business
Page 1 of 1 (39 items)
Powered by Community Server (Non-Commercial Edition), by Telligent Systems