I've been (re)reading Clean Code by Robert Martin recently and I've been struck by how poor some of the code examples are. Clean Code is admittedly a classic, and few books have made me think more about the art of programming, but "Uncle Bob" seems to be better at writing about code than, well, writing code.
This is Listing 2-1, one of the first examples in the book.
This gets refactored into Listing 2-2:
This code isn't just not very good, it's actually worse than the original. What's wrong with it? Let me count the ways...
The biggie with this code is that Martin is introducing state into his program where there is no need to do so. I'm guessing Martin is not a fan of functional programming! Three of his methods set instance variables rather than returning values. This is a possible source of bugs if methods are called in the wrong order; possibly not an issue with a class this simple, but still better avoided.
More seriously, state causes issues with concurrency. What happens if another thread enters createPluralDependentMessageParts() when a previous thread is still completing make()? Martin might protest that he was writing this method as part of a single-threaded program, but who knows what that code might be used for in the future? You can't plan for every eventuality, but creating state where there's no need for it is just asking for trouble.
On the subject of that method, createPluralDependentMessageParts(): yuck! It's not the world's worst name -- it does actually say what it is literally doing -- but it's terribly awkward. It takes a little mental processing to work it out.
Next, Martin's code is not very orthogonal. What happens if we decide, for reasons of aesthetics, to add an apostrophe, or an "e", before the plural "s"? (Leaving aside the question of correctness of punctuation here!) In Martin's new code, this would have to be changed in two different methods. Not very orthogonal, and worse, the same change would have to be made: oops, we're violating the One Point of Authority principle.
Finally, Martin's code is longer than the original, both in line count and number of methods (from one to five). Usually when I've improved some code the refactored version is smaller and simpler than the original. Of course, if increasing the number of lines makes the code more correct, or easier to read, then no problem: it's a win. But if everything else is equal (or approaching equal) then more LoC is less quality. Each extra line is a possible source of bugs, extra complexity, and is more to read through or grep through.
So, how would I write the method? I'm glad you asked... well just for comparison, this is how I'd probably write it:
This is less complicated than Martin's version, easier to read (in my opinion), does not introduce state, and is more orthogonal (you can see how changing the plural form would be easy in my version). As a bonus, it is shorter than the original, and much shorter than Martin's version.
I have used the ternary operator liberally, which is not to everyone's taste, but the above all applies if you translate my version into if statements.
If you forced me to write according to Martin's philosophy (e.g. taking "one method, one action" as far as it can possibly go, putting this in its own class; all of these ideas have a fair amount of merit), then I'd come up with something like this:
This is Listing 2-1, one of the first examples in the book.
This gets refactored into Listing 2-2:
This code isn't just not very good, it's actually worse than the original. What's wrong with it? Let me count the ways...
The biggie with this code is that Martin is introducing state into his program where there is no need to do so. I'm guessing Martin is not a fan of functional programming! Three of his methods set instance variables rather than returning values. This is a possible source of bugs if methods are called in the wrong order; possibly not an issue with a class this simple, but still better avoided.
More seriously, state causes issues with concurrency. What happens if another thread enters createPluralDependentMessageParts() when a previous thread is still completing make()? Martin might protest that he was writing this method as part of a single-threaded program, but who knows what that code might be used for in the future? You can't plan for every eventuality, but creating state where there's no need for it is just asking for trouble.
On the subject of that method, createPluralDependentMessageParts(): yuck! It's not the world's worst name -- it does actually say what it is literally doing -- but it's terribly awkward. It takes a little mental processing to work it out.
Next, Martin's code is not very orthogonal. What happens if we decide, for reasons of aesthetics, to add an apostrophe, or an "e", before the plural "s"? (Leaving aside the question of correctness of punctuation here!) In Martin's new code, this would have to be changed in two different methods. Not very orthogonal, and worse, the same change would have to be made: oops, we're violating the One Point of Authority principle.
Finally, Martin's code is longer than the original, both in line count and number of methods (from one to five). Usually when I've improved some code the refactored version is smaller and simpler than the original. Of course, if increasing the number of lines makes the code more correct, or easier to read, then no problem: it's a win. But if everything else is equal (or approaching equal) then more LoC is less quality. Each extra line is a possible source of bugs, extra complexity, and is more to read through or grep through.
So, how would I write the method? I'm glad you asked... well just for comparison, this is how I'd probably write it:
This is less complicated than Martin's version, easier to read (in my opinion), does not introduce state, and is more orthogonal (you can see how changing the plural form would be easy in my version). As a bonus, it is shorter than the original, and much shorter than Martin's version.
I have used the ternary operator liberally, which is not to everyone's taste, but the above all applies if you translate my version into if statements.
If you forced me to write according to Martin's philosophy (e.g. taking "one method, one action" as far as it can possibly go, putting this in its own class; all of these ideas have a fair amount of merit), then I'd come up with something like this:
Again, there's no state but the program is more modular. I'm very often in favour of breaking methods down into smaller methods but at some point you can take it a bit too far and lose the benefits. That point is in between the two code fragments I've written.
Although I don't like some of his code, the rest of Martin's book is rammed with useful tips and (thought-) provoking ideas. Just don't take it all as gospel!
