Sandi, along with Katrina Owen, is working on a book about OO Design based on the “99 Bottles of Beer” song. If that coding problem is good enough for them, then it’s good enough for me! Exercism offers it as one of their problems as well, so we already have a working test suite.
Just hacking code together, I ended up with this solution.
Now that our tests pass we can ask, “How TRUE is our code?” That will be easier to determine in the context of some new features:
- California is clamoring for Beer Song as a Service (BSaaS), but they want it about wine
- Because wine is stronger, the full song should start with 20 bottles, not 99
- Also, they will bring up more wine from the cellar, not buy it at a store
It is easy to see the code’s function and the effect of a change
sing is pretty transparent.
verses has a bit of rigmarole to handle Ruby’s ranges only being ascending, not descending. But it’s idiomatic enough that I understand it. The
verse method isn’t as transparent as I’d like. Its if/elsif/else structure uses some magic numbers and also makes it hard to see the differences between the cases. But its function isn’t totally opaque.
Is the code we need to change to implement these features transparent to us? I think so. In a new
WineSong class we’d change all instances of the word “beer” to “wine” and we need to change the special “no bottles” verse to say that they go to the cellar
Then we’d change
sing need to call
But then, oops, we see that the 99 is also in the special “no bottles” verse, so we have to change it there. So maybe that change is not as transparent as we thought.
The effort to make a change reflects its complexity
All of these features are very low-complexity changes. Do they all take the same amount of effort? Not really. The cellar wording requires us to change a couple of words, easy. Going from 99 verses to 20 requires a change to a method and to the wording, so is a little tougher. But changing “beer” to “wine” requires us to change the code in 12 places. Yes, you can use Find and Replace (or your tool of choice) to make that change in one command. But you still end up changing the code in 12 places for a one word change. That seems excessive.
So, for the changes we want to make, our code is not entirely Reasonable.
The code can be used in other contexts
This one is pretty clear. We have code that works in a Beer context, but it requires multiple changes to work in a Wine context. The only unchanged code is the
verses method. The other two methods in BeerSong needed changes to work in the Wine context. This code is not very Usable.
The code serves as an example for future code
Let’s look at our we’d implement WineSong without changing BeerSong.
Because of the way BeerSong was structured we had to duplicate 95% of it in order to implement WineSong. Now imagine we implement TeaSong and MonsterEnergyDrinkSong, etc. This code will be copied each time. That is not exemplary.
My code is kind of transparent, kind of reasonable, not usable and not exemplary. What is the next step? Like we did with the 4 Rules, let’s just start at the beginning and try to improve our code.
The big problem we had with Transparent was that changing the
sing method revealed an unexpected necessary change in the
verse method. Let’s fix that.
I don’t tend to use constants, but it works here and solves our Transparency problem. In Refactoring, this approach is closest to “Replace Magic Number with Symbolic Constant” (p. 204).
This change also fixes a problem that cropped up under Transparent and Reasonable – changing the verses from 99 to 20 required a change to both
verse. Now if we copy this code into a new WineSong class the number of bottles sung in the last verse will be correct. We’re left with just one Reasonableness problem: changing ‘beer’ to ‘wine’ requires 12 code changes. We can follow the same pattern to fix this.
And while we’re at it, let’s take the same approach to the cellar/store problem
After taking the simplest possible approach to make our code more reasonable, our BeerSong now looks like:
We’ve worked our way through T and R, but is our code more Usable as a result? Remember that our usability problem was that we had to clone this entire class in order to make a WineSong. Is that still the case? Well, maybe we could do this:
We can’t actually do that, though. Redefining constants is both a bad idea and justifiably trickier. But the fact that we have isolated just these few differences between our Beer and WineSongs shows us that we’re on the right Usability track, we just need to try a better solution.
Before we try another solution, I’ll tell you a little secret: Sandi’s “Usable” metric is just a stealthy way of saying “Open/Closed Principle”. Code that is “usable in other contexts” is code that is “open to extension”. I’ve covered the Open/Closed Principle before and how to refactor our code so that it follows OCP:
- Find the code that makes it hard to implement your new feature
- Refactor your existing code to remove that difficulty
- Extend your newly refactored code with the new feature
We’ve already done the first step and found the five variables that prevent us from easily implementing WineSong. Let’s move on to the second step. If we have variable that need to change between types, then some Extract Class (Refactoring, p. 149) and inheritance could work.
Or maybe you want something more Factory-like.*. Then you can Replace Inheritance with Delegation (Refactoring, p. 352).
And now we’ve separated the algorithm of the song from its configuration, meaning that the DrinkingSong can be used in any drinking context (well, anything that comes in bottles; though that dependency would be easy to extract).
We can check U off our list and look at Exemplary. When I reviewed the initial version of this code I said it was Unexemplary because any additional songs required a near-exact copy of the BeerSong class. But, hey, we just solved that problem! There might be some things we want to tweak with our new solution, but it raises far fewer alarms than our old code. I would be ok with the factory-like solution being used for MonsterEnergyDrinkSong, or whatever other song comes our way.
There’s still one outstanding problem – the if/elsif/else-happy
verse method with and its duplication that we noticed earlier. As we saw when we used Open/Closed, code that doesn’t cause problems doesn’t get refactored. Nothing compelled me to change this method, so I didn’t. There certainly seems to be a lot of duplication in
verse, but later features might reveal that this is Incidental Duplication, not duplication of knowledge. Unlikely, but possible. Point is, it doesn’t matter right now, so let’s quit before we make the code worse.
You do not have to wait to find out what this week’s newsletter will be about, I’ll just tell you; I’ll be talking about one of the most prevalent code smells in the Ruby-verse – Long Parameter List. You can sign-up for the newsletter, check out previous issues or go sledding/surfing (depending on hemisphere). Comments/feedback/&c. welcome on twitter, GitHub or firstname.lastname@example.org
* I also promised slightly-dirty nursery rhymes, which I ended up not writing about.
* This may not be an official factory, I need to spend more time with the Patterns book.