Believing in booleans



  • Here's an anti-pattern I've seen at every company I've worked for in the last 20 years, in C, C++, Java, C#, and python:


    if (myBoolean == true)
    {
    // do something
    }

    There is a variant:

    if (val > 200)
    {
    myBoolean = true;
    }
    else
    {
    myBoolean = false;
    }

    ...instead of

    myBoolean = (val > 20);

    Maybe I've got too much time on my hands, maybe I'm a crochety old SOB, but his bugs me.



  •  There's a hiring ad from some company here that goes along the lines of:

    (boolValue? true : false)

    Does code like this annoy you as well? Come talk to us!

    It's kind of cool.


  • BINNED

    I've known people who do this. They actually think it's better because it makes what the code is doing more explicit.



  • While I like my code explicit, I don't like it making uneeded checks I know are already being done.  So I will wrap code in brackets when not always needed to clearly define the boundaries and also save myself from arbitraily adding lines that change logic later, but I won't check myValue == True.

    i.e.:

    if(isBlue)

    {

         //single line Do something;

    }



  • Another one I see a lot is:

    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.



  • @morbiuswilters said:

    Another one I see a lot is:

    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }


  • I agree, don't invert your check, invert the choice instead.

    It is only natural to read the first part of an if statement as a "value is true" path.  The only time I deviate from this is if the false side is just a few lines when the true side is fairly large, just so the few lines of code don't get lost down below somewhere.

    Once again this is personal style, sytatically there is nothing wrong with it, but it just doesn't always feel right.



  • @pkmnfrk said:

    @morbiuswilters said:
    Another one I see a lot is:

    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }

     

    +1

     



  • @pkmnfrk said:

    @morbiuswilters said:
    Another one I see a lot is:

    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }

    Ugh, why? Assuming the error cleanup is what I think it is, I'd do:

    if (isErrorCondition) {
      // clean up our mess
      return;
    }
    
    // do useful work

    It's a lot easier to read and avoids unnecessary nesting.



  • @morbiuswilters said:

    @pkmnfrk said:
    @morbiuswilters said:
    Another one I see a lot is:
    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }
    Ugh, why? Assuming the error cleanup is what I think it is, I'd do:
    if (isErrorCondition) {
      // clean up our mess
      return;
    }
    

    // do useful work


    It's a lot easier to read and avoids unnecessary nesting.

    PK makes me think he is emulating a try catch pattern, why not just use try catch finally, unless your language doesn't have it?

    Morbs, yours leads me to think there may be other if(errorconditions) set up all through the function with multiple returns scattered therein, to me this is just another bad practice.



  • In a COBOL class I took long, long ago, the old fart teaching the class absolutely insisted that you never check for inequality, thus leading to idiotic constructs like this:

    if (something) {

         // do nothing

    }

    else {

         // do something

    }

    Hooray for zero-tolerance shop standards.



  • @KattMan said:

    Morbs, yours leads me to think there may be other if(errorconditions) set up all through the function with multiple returns scattered therein, to me this is just another bad practice.

    What? I never said anything about duplicating the error handling code. If you're referring to multiple returns being bad practice, then you are being straight-up retarded. "Quick, let's write all of our code as a deeply-nested tangle of ifs and elses just so we can have a single return! It's soooooo confusing when a function might return anywhere except at the very bottom!!" Seriously, that has to be the dumbest anti-pattern I have ever encountered.



  • @morbiuswilters said:

    Ugh, why? Assuming the error cleanup is what I think it is, I'd do:

    if (isErrorCondition) {
      // clean up our mess
      return;
    }
    

    // do useful work

    It's a lot easier to read and avoids unnecessary nesting.

    Ah, the "Guard Pattern" ... I use this a lot ...

    My first non-BASIC structured language I learned was Pascal back in high school (ca 1982) [ahhh, Apple Pascal ... but I digress]. The single-entry/exit pattern was en-vogue and that's what I learned ... $diety what messy, ugly hard-to-understand code that was. I still find myself doing it in internal if/then constructs and try to break myself of the habit.



  • @rstinejr said:

    myBoolean = (val > 20);

    Yay! It's not just me that actually does code like that! (Sadly in my team, I think it is just me, and lines like that do confuse my comrades....)



  • @morbiuswilters said:

    @KattMan said:
    Morbs, yours leads me to think there may be other if(errorconditions) set up all through the function with multiple returns scattered therein, to me this is just another bad practice.
    What? I never said anything about duplicating the error handling code. If you're referring to multiple returns being bad practice, then you are being straight-up retarded. "Quick, let's write all of our code as a deeply-nested tangle of ifs and elses just so we can have a single return! It's soooooo confusing when a function might return anywhere except at the very bottom!!" Seriously, that has to be the dumbest anti-pattern I have ever encountered.

    There you go assuming again and putting words in someone elses mouth.

    I know you never said you woudl be duplicating the error handling code, I said it implied that there might be more error conditions present.

    As such:

    if(errorCondition)

    { //do somthing and return}

    //Do some work

    if(nextErrorCondition)

    {//do somthing and return}

    //do some more work

    if(nextErrorCondition)

    {//do somthing and return}

    etc....

    Like I said it implies it, it doesn't mean you are doing it, but if you are I see it as bad practice.  Tells me your functions are probably doing far to much work in one place, refactor it.



  • @KattMan said:

    There you go assuming again and putting words in someone elses mouth.

    You're the one who looked at my single, hypothetical if statement and jumped the conclusion that the function must contain numerous error-checking if statements and then went further and assumed that these should (and could) be logically separated. Who's putting words in whose mouth?

    @KattMan said:

    Tells me your functions are probably doing far to much work in one place, refactor it.

    Oh good, coding "standards" that have no basis in reality, I love those. Since this is a hypothetical function (one that you mostly invented in your own head, BTW) I find it really difficult to say that it's "doing too much". This sounds like those morons who say no function should be more than 25 lines. Rather than mechanically apply contrived standards, I tend to look at actual code and engage my critical thinking abilities to determine if the code can be cleanly separated to make it more readable and maintainable.



  • @morbiuswilters said:

    If you're referring to multiple returns being bad practice, then you are being straight-up retarded. "Quick, let's write all of our code as a deeply-nested tangle of ifs and elses just so we can have a single return! It's soooooo confusing when a function might return anywhere except at the very bottom!!" Seriously, that has to be the dumbest anti-pattern I have ever encountered.

    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.



  • @KattMan said:

    @morbiuswilters said:

    @pkmnfrk said:
    @morbiuswilters said:
    Another one I see a lot is:

    if (!isSomething) {
    // do stuff
    } else {
    // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }
    Ugh, why? Assuming the error cleanup is what I think it is, I'd do:
    if (isErrorCondition) {
      // clean up our mess
      return;
    }
    

    // do useful work


    It's a lot easier to read and avoids unnecessary nesting.

    PK makes me think he is emulating a try catch pattern, why not just use try catch finally, unless your language doesn't have it?

    Morbs, yours leads me to think there may be other if(errorconditions) set up all through the function with multiple returns scattered therein, to me this is just another bad practice.

    I wasn't, that was a bad example. My point was that the first block should be the primary emphasis of the code, while the second block should be the secondary emphasis. It's like in Perl or other languages where this is a thing:

    doSomething() or die();

    vs

    die() if !doSomething();

    (I have never written Perl, so that second example may not be syntactically valid)



  • @Smitty said:

    @morbiuswilters said:
    If you're referring to multiple returns being bad practice, then you are being straight-up retarded. "Quick, let's write all of our code as a deeply-nested tangle of ifs and elses just so we can have a single return! It's soooooo confusing when a function might return anywhere except at the very bottom!!" Seriously, that has to be the dumbest anti-pattern I have ever encountered.

    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.

    I once had a "Senior Java Architect" tell me that Java can't return from the middle of a function because "the compiler can only find return statements if they're at the bottom of a function". He was astounded when I showed him that wasn't true. This gives you an idea of the average intelligence of people with the title "Senior Java Architect".



  • @pkmnfrk said:

    doSomething() or die();

    Yes, I despise this. Same as:

    isCondition && doSomething();


  • @morbiuswilters said:

    "the compiler can only find return statements if they're at the bottom of a function"

    My guess as to how this came to be:

    Instructor: You will typically find return statements at the bottom of functions.

    SJA's brain: You ... find return ... bottom ... functions.

    SJA's notes: You... something something... return... something... bottom of functions.

    SJA, three weeks later: return statements are only found at the bottom of functions. Why? Well, um, the compiler isn't going to look anywhere else, right?



  • @rstinejr said:

    Here's an anti-pattern I've seen at every company I've worked for in the last 20 years, in C, C++, Java, C#, and python:
    if (myBoolean == true)
    {
    // do something
    }

    In C#, sure, that's not necessary, but C/C++ don't have an actual bool value, so explicit comparison can get rid of bugs.


  • @morbiuswilters said:

    @KattMan said:
    There you go assuming again and putting words in someone elses mouth.
    You're the one who looked at my single, hypothetical if statement and jumped the conclusion that the function must contain numerous error-checking if statements and then went further and assumed that these should (and could) be logically separated. Who's putting words in whose mouth?

    @KattMan said:
    Tells me your functions are probably doing far to much work in one place, refactor it.
    Oh good, coding "standards" that have no basis in reality, I love those. Since this is a hypothetical function (one that you mostly invented in your own head, BTW) I find it really difficult to say that it's "doing too much". This sounds like those morons who say no function should be more than 25 lines. Rather than mechanically apply contrived standards, I tend to look at actual code and engage my critical thinking abilities to determine if the code can be cleanly separated to make it more readable and maintainable.

    You never fail to amaze me.  You snip and edit to prove your own point ignoring what was really said.

    Once again I did not say you were doing this and I did not say that it must contain numerous error checkign statements, I said the pattern implied that it could, and IF it did it would require looking at things a bit closer.  If it can not be "mechanically separated" then it is doing one thing.  As far as my statement about refactoring, would you prefer a function 1000 lines long that is trying to do 10 different hings all at once?  I think not.  I have encountered those and they are debugging nightmares, refactor them into logical separations.

    Also never once did I mention anything about a preferred length, only that it should try to do one logical process, if that process does take a few hundred lines, so be it, but trust me, I will look at it and see if something can be cut down.  I do know there are times functions HAVE to be longer based off what they are doing, anything with a switch usually comes to mind as an easy example, just try to follow a hard core 25 lines only rule when you have to do that and you probably can't.  I'm realistic, but also don't like runaway code, if that wasn't a problem, why even call any functions, put it all in main() and be done with it.



  • @rstinejr said:


    Maybe I've got too much time on my hands, maybe I'm a crochety old SOB, but his bugs me.

    Yes



  • @Sutherlands said:

    @rstinejr said:

    Here's an anti-pattern I've seen at every company I've worked for in the last 20 years, in C, C++, Java, C#, and python:

    if (myBoolean == true)
    {
    // do something
    }


    In C#, sure, that's not necessary, but C/C++ don't have an actual bool value, so explicit comparison can get rid of bugs.
     

     

    Not good enough.  The actual code is:

     if (myBoolean == true)

    {

      // do something

    }

    else if (myBoolean == false)

    {

      // do something else

    }

     



  • @Sutherlands said:

    @rstinejr said:

    Here's an anti-pattern I've seen at every company I've worked for in the last 20 years, in C, C++, Java, C#, and python:

    if (myBoolean == true)
    {
    // do something
    }

    In C#, sure, that's not necessary, but C/C++ don't have an actual bool value, so explicit comparison can get rid of bugs.

    Or, in some cases, be part of a fun-to-find bug:

    #define TRUE 1
    #define FALSE 0
    
    // more flags
    #define FLAG_FOO 0x0080
    // more flags
    
    ...
    
    unsigned int isFooFlagSet = flags & FLAG_FOO;
    
    ...
    
    if (isFooFlagSet == TRUE)
    {
      ...
    }
    

  • ♿ (Parody)

    @Smitty said:

    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.

    I'd start breaking out the goto. Most standards I've been around seem to assume no one would dare go there, and don't explicitly rule them out.



  • I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.



  • @boomzilla said:

    @Smitty said:
    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.

    I'd start breaking out the goto. Most standards I've been around seem to assume no one would dare go there, and don't explicitly rule them out.

    I probably could have gotten away with it. This same shop insisted that 3000-line methods were totally fine, because "a very small call graph makes everything easier to understand".



  • @Sutherlands said:

    @rstinejr said:

    Here's an anti-pattern I've seen at every company I've worked for in the last 20 years, in C, C++, Java, C#, and python:

    if (myBoolean == true)
    {
    // do something
    }

    In C#, sure, that's not necessary, but C/C++ don't have an actual bool value, so explicit comparison can get rid of bugs.

    Not only is it not necessary, it is potentially incorrect as well!

    unsafe void DontEverDoThisOrIWillHuntYouDown() {
        bool myBool = true;
        
        fixed(bool* myBoolPtr = &myBool) {
            int* myIntPtr = (int*)myBoolPtr;
            *myIntPtr = 42;
        }
        
        if(myBool != true && bool != myBool ) throw new Exception("myBool == FILE_NOT_FOUND");
    }
    

    Not tested, but the idea is there.



  • @ubersoldat said:

    I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    But there are times when you need to just toss your results and get out.  Using a construct that wraps the rest of the code just to get to the bottom is overkill.

    My attitude is this, If it is easily understood, does what it says it does nothing more, and actually handles the exceptions, then I say it's probably good to go.



  • @mightybaldking said:

    Not good enough.  The actual code is:

     if (myBoolean == true)

    {

      // do something

    }

    else if (myBoolean == false)

    {

      // do something else

    }

     

    Must be a DBA... gotta watch out for those null booleans!


  • :belt_onion:

    @ubersoldat said:

    Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    ... no, using return in the middle of a loop is for people who know how code flow works and want to return from the function in the middle of a loop.

    I don't believe in making functions do unnecessary work for the sake of prettiness.

     



  • @dhromed said:

    @pkmnfrk said:

    @morbiuswilters said:
    Another one I see a lot is:

    if (!isSomething) {
      // do stuff
    } else {
     // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }

     

    +1

     

    While not the best example I have to agree that there is a place for this. It depends on the logic. Sometimes the inverse makes more sense, but it's rare (in my experience). As a general rule of thumb, yeah don't start with a 'not' block, but as with most things coding-wise you have to use your brain and think of how it works in the particular situation. Following rules for the sake of rules leads to WTF.

    if(!retarded)
    {
        // do useful work 
    }
    else
    {
      // uh oh
    }
    


  • @boomzilla said:

    @Smitty said:
    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.
    I'd start breaking out the goto. Most standards I've been around seem to assume no one would dare go there, and don't explicitly rule them out.

    And easy to argue that GOTO is simply a JUMP, just like a function call, the tail of a loop, and an if statement, JUMP is everywhere so why avoid the use of one!

    ok, sarcasm off.



  • @ubersoldat said:

    I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    You're a fucking idiot. Never seen this?

    void MyMethod() {
        if(!somePrecondition) return;
        if(!somethingElse) return;
    
        //do stuff
    }
    

  • BINNED

    @KattMan said:

    While I like my code explicit, I don't like it making uneeded checks I know are already being done.

    The thing is, the redundant checks don't really make the code more explicit. They just make whoever wrote them look like they don't understand their programming language.



  • Isn't that what I said?  It's a balance at times, and yes sometimes that balance can shift one way or the other, but I try not to get to silly.



  • @pkmnfrk said:

    @ubersoldat said:

    I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    You're a fucking idiot. Never seen this?
    void MyMethod() {
        if(!somePrecondition) return;
        if(!somethingElse) return;
    
    //do stuff
    

    }

    Wow, did ubersoldat piss in your cheerios this morning or do you just have the vocabulary of a 50-year-old with an IQ of 60?



  • @pkmnfrk said:

    @ubersoldat said:

    I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    You're a fucking idiot. Never seen this?
    void MyMethod() {
        if(!somePrecondition) return;
        if(!somethingElse) return;
    
    //do stuff
    

    }

    It could be argued that the preconditions could be checked even before entering the function.

    Now if there was a little setup work first then you checked something else and exited, yes much better then wrapping the rest in an else block for a single return because it is pretty clear what you are doing.  Just don't start checking every few lines in yoru work if you need to return.  This is where the whole "single return rule" I think comes from, a knee-jerk reaction to bad practices with returns scattered all through a function for whatever reason instead of actually checking if you can even start working up front.



  • @pkmnfrk said:

    @morbiuswilters said:
    Another one I see a lot is:
    if (!isSomething) {
    // do stuff
    } else {
    // do other stuff
    }

    It's less readable. I usually invert it when I see it.

    This one is debatable:

    if(!isErrorCondition) {
        // do useful work
    } else {
        // clean up our mess
    }

    It could also be a case where you want the condition that occurs 90% of the time to come first:

    if(!isUnusual) {
        // do typical stuff
    } else {
        // do special-case stuff
    }


  • @ekolis said:

    Must be a DBA... gotta watch out for those null booleans!
     

     

    #define FILE_NOT_FOUND 2.99999999999999

     



  • @KattMan said:

    You snip and edit to prove your own point ignoring what was really said.

    What did I edit? I snipped out your dozens of line of meaningless crap because nobody needs to see it repeated. What's really bizarre is that you seem to think my single, hypothetical line implied anything about the rest of the function. Stop pulling stuff out of your ass.



  •  @pkmnfrk said:

    @ubersoldat said:

    I believe it's good practice to have a single point of exit. This makes it easier when debugging. Using return in the middle of a loop is for people who don't know how code flow works and how to use break and continue.

    You're a fucking idiot. Never seen this?

    void MyMethod() {
        if(!somePrecondition) return;
        if(!somethingElse) return;
    
        //do stuff
    }
    

     

    I call that the Quit While You're Ahead pattern in recursion cases and trivial cases and the GTFO pattern in error cases.

    int Factorial(int num)

       if (num <0) return ERROR_CODE; // GTFO error pattern (an exception is probably a better choice.) 

       if (num == 0 || num ==1 )  return 1; // Quit while you're ahead

       return num * Factorial(num - 1);

    }

     

    Yes, That's 3 returns (although, I'd probably check the error in a public method, and then pass to a private recursive method to do the actual work.)

     I'm not sure that re-arranging that code to have a single return makes the code any more readable or understandable.  

    My general rule is to use return as soon as I have calculated what to return, barring any cleanup necessities.

     



  • @heterodox said:

    I don't believe in making functions do unnecessary work for the sake of prettiness.

    The worst part is, it's not even pretty. Single-return is just an ugly anti-pattern promulgated by stupid people who don't understand flow control and who can't be bothered to read code. Seriously, the thinking is "I can't return here, I better set a variable and then break out of the loop so the end of the function can execute that return for me."


  • ♿ (Parody)

    @mightybaldking said:

    I'm not sure that re-arranging that code to have a single return makes the code any more readable or understandable.

    I've generally thought that this rule was similar to the "short function" rule. Basically, a proxy for not writing crazy functions that are very difficult to follow. So if you've written a 300 line function, you should consider that it could probably be made easier to read and follow by breaking it up. And it's those sorts of functions, IME, that have returns stashed all over the place.



  • @boomzilla said:

    @mightybaldking said:
    I'm not sure that re-arranging that code to have a single return makes the code any more readable or understandable.

    I've generally thought that this rule was similar to the "short function" rule. Basically, a proxy for not writing crazy functions that are very difficult to follow. So if you've written a 300 line function, you should consider that it could probably be made easier to read and follow by breaking it up. And it's those sorts of functions, IME, that have returns stashed all over the place.

     

     

    Yes, that's actually a good point. A couple of returns in the middle (not tip) of the notorious "Arrow Pattern" could make it nearly impossible to figure out.

     



  • @pkmnfrk said:

    die() if !doSomething();

    (I have never written Perl, so that second example may not be syntactically valid)

     

    It is correct, but this is more fun:

    die() unless doSomething();

    :)



  •  And let's not forget this little gem:

    if( someBoolean == true )

    {

        return true;

    }

    else

    {

        return false;

    }

    ...following the school of thought that the more code you write, the clearer it gets.



  • @morbiuswilters said:

    ... If you're referring to multiple returns being bad practice, then you are being straight-up retarded. "Quick, let's write all of our code as a deeply-nested tangle of ifs and elses just so we can have a single return! It's soooooo confusing when a function might return anywhere except at the very bottom!!" Seriously, that has to be the dumbest anti-pattern I have ever encountered.

    @Smitty said:

    A single return at the bottom was the shop standard at my last job. I was shot down and told I was 'inexperienced' at a staff meeting when I dared to suggest that guard clauses might be preferable to Ifs nested six deep. Fuck that place.

    @morbiuswilters said:

    The worst part is, it's not even pretty. Single-return is just an ugly anti-pattern promulgated by stupid people who don't understand flow control and who can't be bothered to read code. Seriously, the thinking is "I can't return here, I better set a variable and then break out of the loop so the end of the function can execute that return for me."

    I figured this one out, its a case of igoring the context of a best practice. The single return policy is only a best practice when using C & Malloc/Free, in a context with say RAII, or in a managed environment it is an anti-patern. If you are unlucky enough to have to maintain that kind of C code, you know you have to follow the single return policy or it will bite you in the ass!

Log in to reply