Doc Norton & Associates

View Original

I disagree with Uncle Bob on this one

I am reading Bob Martin's "Clean Code - A handbook of Agile Software Craftsmanship" again. For the most part, I am really digging it. It reminds me a bit of Steve McConnell's Code Complete. I still have the original version of McConnell's book around here somewhere. But I digress (and show my age).

In Chapter 9, Martin covers unit tests. In listings 9-3 and 9-4, Martin shows the "simplification" of a unit test. Listing 9-3 makes two method calls followed by five assertions. Each assertion checks a specific state. Each state is clearly identified. I do not particularly like the five assertions. But this is not the point that Bob makes. This is not the correction he covers. Instead, he condenses all five assertions into a single assertion by introducing a string of seemingly arbitrary characters.

We go from:

assertTrue(hw.heaterState());
assertTrue(hw.blowerState());
assertFalse(hw.coolerState());
assertFalse(hw.hiTempAlarm());
assertTrue(hw.loTempAlarm());

To:

assertEquals("HBchL", hw.getState());

He goes on to explain that each character in our string represents a possible state; Capital is true, Lower is false. He follows up with, "Notice, once you know the meaning, your eyes glide across that string and you can quickly interpret the results. Reading the test becomes almost a pleasure."

WHAT?

"Once you know the meaning"? This is not from a solution domain. This is not from the problem domain. This removes clarity under the guise of improved readability. How, pray tell, am I to know the meaning? Is there a comment in the code to explain the meaning? Is he really suggesting that a blatant code smell is better than methods that clearly indicate the intended behavior? And what happens when getState needs to also return the humidifier State? How much code breaks? I should be able to introduce a new behavior without these issues.

I have to disagree with Uncle Bob on this one. This is NOT a better solution. It does NOT make the code more readable. I am suprised to see this recommendation and I bet it does not appear in the Second Edition.

Break each assertion out into their own test within the class, put them all into their own test class with a shared setup and tear-down, or use a Template Method pattern. Frankly, you could just leave them all in a single test given they all test the same single concept. But this string of place-holders with case representing on or off state is a fart in a garden of beautiful code. It just smells bad.