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.
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.
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.
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.
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.
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.)
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.
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.
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.
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!
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?
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).
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….
I prefer…..
Sometimes if there is nothing to be done in the else case I will explicitly go…
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?
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.
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).
eg. Rust. I’ll admit that is the number one feature of Rust that I’d like to see in D.
I go the other way: anywhere I see myself using an
ifI 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.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.
To my mind using ifs that way is a step in the wrong direction. If what we actually want is:
then
is to my mind closer to that than
- 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.
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…
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.)
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.Scala, FWIW.
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.
Yep, absolutely. But to my mind the step just before that is to approximate
VideoDataas 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.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
b1andb2functions if the conditionals are a little hairy, thus avoiding boolean variables, which always feel off to me, for some reason.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.
I started doing this after reading your comment 11 months ago (including writing explicit empty
elseclauses 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 nestedif/else’s). Thank you!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?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…
Which would allow me to test the authorization logic separately from shooting things.
Which would be a Very Good Thing.
Just curious, why did you use
bool_tinstead ofboolSorry, typo.
Thanks for the reply. That makes sense, thanks for the tip. I think I’ll start doing that now.
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!
I would rather call the method
shootIfAuthorizedor 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).