1. 11
  1.  

  2. 8

    Mostly I agree with what he says, except this.

    My general rule of thumb is never use a boolean flag.

    You’re only shifting the place where you do the if() from the place where you set the flag, to somewhere else.

    Usually it’s better to do the if() right there where you set the flag.

    Instead of decomposing the boolean expression into subexpressions, decompose it into sub if’s… Gives you a chance to explicitly think through whether you have handled every branch.

    ie. Instead of….

    b1 = expression1
    b2 = expression2
    if( b1 && b2) ....
    

    I prefer…..

    if( b1) {
        if( b2) {
            ... // What he did...
        } else {
            ... Did he think about this case?
        }
    else {
       ... And these two cases b2 and !b2?
    }
    

    Sometimes if there is nothing to be done in the else case I will explicitly go…

    if( b2) {
       ...
    } // else not needed because....
    

    Yup, my way is way more verbose and branchy….

    Except his way hid the branches, they are there implicitly, but his code doesn’t invite you to consider each one individually for sanity.

    In production each one of those branches will execute. Have you explicitly thought each possibility through?

    When your successor revisits the code after making a change, will he think through each branch? Or gloss over the other three cases?

    1. 5

      I think this can get out of hand, that being said, I can see the benefit.

      Languages with pattern matching are like this on steroids where the compiler checks that you have matched exhaustively.

      1. 5

        Yes, it gets out of hand very rapidly.

        But I’d argue that that Is A Good Thing.

        It slaps you in the face that your cyclomatic complexity is huge and hence bug prone and hence needs either simplification and/or excellent test coverage.

        And often the only way to get that test coverage is simplification (eg. Extract method).

        Languages with pattern matching are like this on steroids where the compiler checks that you have matched exhaustively.

        eg. Rust. I’ll admit that is the number one feature of Rust that I’d like to see in D.

      2. 4

        I go the other way: anywhere I see myself using an if I ask whether I can replace it with polymorphism (one usually can). Branching is just one special case of a value having behaviour, and it’s too powerful/abstract for most use cases. Usually when you want to branch it’s because you have to deal with two different varieties of thing - better to represent that as two different varieties of thing.

        1. 1

          Correct.

          That’s step 2.

          Replace boolean flag with if() is step 1.

          Step 2 is consider replacing the if() with polymorphism…. but let that choice be ruled, note I say ruled, not guided, but ruled, by the Liskov Substitution Principle.

          1. 1

            To my mind using ifs that way is a step in the wrong direction. If what we actually want is:

            def playVideo(vd: VideoData) = {
              val x = vd.decode()
              ...
            }
            
            playVideo(...)
            

            then

            def playVideo(data: Array[Byte], isMpeg: Boolean) = {
              val x = if(isMpeg) decodeMpeg(data) else decodeDivx(data)
              ...
            }
            
            playVideo(...)
            

            is to my mind closer to that than

            def playMpeg(data: Array[Byte]) = {
              val x = decodeMpeg(data)
              ...
            }
            def playDivx(data: Array[Byte]) = {
              val x = decodeDivx(data)
              ...
            }
            
            if(...) playMpeg(...) else playDivx(...)
            

            - as long as we keep the boolean flag with the data that it goes with we’re halfway to where we want to be, then we just have to introduce a parameter object.

            I generally believe that pre- and postconditions and invariants should be expressed in the type system and mutability should be an explicitly managed effect rather than part of values, in which case the LSP becomes trivial.

            1. 1

              Except it is around about the point where you evaluate the expression that will become isMpeg where you have to decide whether to construct a MpegData or DivxData.

              Not sure which language you’re speaking, so I will use pseudo ruby….

              It’s that point where you can fix the structure, not in play.

              ie. The next step is…

              class VideoData
                 def play
                     val x = decode
                   ...
                 end
              end
              
              class DIVXData < VideoData
                 def decode
                 end
              end
              
              class MpegData < VideoData
                 def decode
                 end
               end
              
              if(...) 
                  vd = MpegData.new 
              else 
                  vd = DivxData.new
              
              vd.play
              

              The actual construction will probably be done in the factory that reads the data and just returns a playable.

              (Also not sure about the play/decode split, just following the structure of your example.)

              1. 1

                Except it is around about the point where you evaluate the expression that will become isMpeg where you have to decide whether to construct a MpegData or DivxData.

                Absolutely. When I’m being purist about this I say the only place where ifs are allowed in my code is the interfaces with external systems/data - and only when deciding what to construct. After that everything is just values, and any branching should happen via polymorphism.

                Not sure which language you’re speaking, so I will use pseudo ruby….

                Scala, FWIW.

                It’s that point where you can fix the structure, not in play.

                Completely agreed - so one vital step along the way is to make sure we evaluate the boolean condition at the point where we read the data and then just pass it through, not do the logic of figuring out which it is later on.

                ie. The next step is…

                Yep, absolutely. But to my mind the step just before that is to approximate VideoData as a pair (Array[Byte], Boolean), and the step just before that is passing it as two arguments. The conditionalness is going to be data - turning it into control flow is a step backwards.

        2. 4

          To be fair, it was just a code snippet, but your point is a good one.

          Also, if the language supports it, I like to make b1 and b2 functions if the conditionals are a little hairy, thus avoiding boolean variables, which always feel off to me, for some reason.

          1. 3

            When I started programming decades ago, I wrote a program that passed boolean flags all over the place.

            After a particularly hard nights debugging, I got fed up. Where and How was I using these things?

            Ultimately if() statements…. What if I shifted the if()’s to where I created them? What would happen to my code? Hmm. I need to make a few new channels of communication, but a largish bunch of “nils” and unused/dummy (in certain cases) parameters went away…..

            …. and Hey! My program is getting smaller! Much simpler! And ooo look the bug vanished and this makes so much more sense.

          2. 2

            I started doing this after reading your comment 11 months ago (including writing explicit empty else clauses with a comment). I was initially sceptical, but I’ve come to really like this approach after some time. It does expose the complexity, and sometimes makes it apparent that there is an extra case that needs to be handled (that usually happens when I refactor older code with complex conditional expressions into nested if/else’s). Thank you!

            1. 1

              I have written on this point specifically. If I understand correctly, you are speaking against if (a && b), not against using variables to clarify your code, correct?

              1. 2

                I will gladly use variables to clarify my code, but booleans are a special case.

                You only use them in other boolean expressions or if()’s.

                I would rather create pure inline functions than variables, since they are more testable / reusable.

                Or nested if’s rather than complex boolean expressions, because I get to think about each branch.

                In your particular example I would actually write a function…

                bool_t User::isAuthorizedToShoot( const Situation& s) const;
                

                Which would allow me to test the authorization logic separately from shooting things.

                Which would be a Very Good Thing.

                1. 1

                  Just curious, why did you use bool_t instead of bool

                  1. 1

                    Sorry, typo.

                  2. 1

                    Thanks for the reply. That makes sense, thanks for the tip. I think I’ll start doing that now.

                    1. 1

                      I have observed a fairly reliable indicator of Good Programmers.

                      They think about, and write about, and get feedback on why they do things.

                      They can tell you why, they personally, think what they do is Good.

                      And they can be convinced, occasionally, to change their minds.

                      I often don’t agree with them, That’s OK.

                      But I find their code is usually much better than those who can’t provide any basis beyond “that’s the way it’s done” or “I just prefer this way”.

                      Keep Blogging!

                  3. 2

                    I would rather call the method shootIfAuthorized or similar. Variables are implementation details; interfaces are the part that needs to be self-documenting (and conversely if something needs to be documented then it should be an interface, since interfaces are also the test point and anything that needs documenting probably needs testing - so I tend to end up with lots of small methods).