In this post, I’ll be working on code to receive batches of CDs into the warehouse from record labels.
There are three examples for this use case in my test list:
- Receive batch of CDs
- Of existing title in catalogue
- Of new title (add to catalogue)
- Of multiple titles
In the first example, we’re receiving copies of a CD that’s already in our catalogue, so we would expect to add those copies to the existing stock. Let’s write a failing test for that.
To pass this test, we just need to search the catalogue for a matching CD and then add the copies to its stock.
Now our test is passing, it’s time for a mini code review. Catalogue now breaks our principle that classes should only do one job. I can easily imagine wanting to change how we receive CDs independently of how we search the catalogue, so this class needs to be broken up.
I’ve extracted a containing class that uses the catalogue, and updated the test to change the target of the receive() method call.
There’s another thing that’s bugging me about the receive() method – it accesses CD’s data directly to add copies to the stock. Although that isn’t Feature Envy, strictly speaking, it most certainly isn’t encapsulation. Let’s fix that.
With that fixed, we can move on to the next test – receiving copies of a CD that isn’t in the catalogue.
To pass this test, we need to check if the CD’s not in the catalogue (i.e., search() returns null), create a new CD if it isn’t and add it to the catalogue before adding the received copies to its stock.
We’re on a green light again, which means it’s time for another mini code review. The first thing that jumps out at me is the distinctly unencapsulated way Warehouse adds the new CD to the catalogue. Let’s fix that.
The next thing that catches my eye is the hardwired dependencies on CD and Description. Now, objects have to be created somewhere, so some code has to use new. But if our goal is to have objects use each other polymorphically, then I have a simple rule: objects that create instances of other classes shouldn’t use them, and objects that use instance of other classes shouldn’t create them. You can create an instance or use an instance, but not both.
It’s also good practice to maintain object creation in one place (D.R.Y.), and by itself so changing – say – the constructor of CD doesn’t impact any other logic.
In fact, we already have a candidate for where that might be: the add() method I extracted and moved to Catalogue. We could move object creation to there and return the new CD to Warehouse.
This simplifies Warehouse and removes its direct dependencies on the implementations of CD and Description. But it just pushed our problem down the call stack to Catalogue, which now has those direct dependencies.
This breaks my swappability rule: Catalogue creates an instance of CD, and it uses CD (by calling cd.matches()). Let’s extract the creation into its own factory method.
And then let’s move that factory method into its own class that we can inject into Catalogue so it doesn’t need to have those hardwired implementation dependencies.
(And, yes, I’m not happy with a class name ending in “Factory”, either. Suggestions for a better name on a postcard, please.)
Some of you may be thinking “Doesn’t Catalogue have two jobs now?” Arguably, yes: searching the contents for CDs and adding CDs to the contents. But both jobs revolve around the same data. If we split these responsibilities apart, we’d have to unencapsulate the contents. Eliminating one code smell will introduce another. It’s very much in the nature of software that we’ll have to make design compromises – we can’t tick all of the boxes all of the time. I’m going to leave this as it is fr now, and we’ll see how the design evolves.
One thing that has occurred to me at this point is: what happened to the CD’s price – where is that set? There’s an extra test here we need to add.
To pass this test, we add price to our factory method.
And pass that data through from Warehouse.
Back on a green light, time for a mini code review. Not much has changed in our source code. Two things strike me; first, we have a Data Clump emerging, with the parameters title, artist and price now appearing in several method signatures. Also, three parameters is right on the edge of what I’d consider to be a Long Parameter list. Making that last test pass illustrated why this can be a problem; I found myself adding the same parameter multiple time to multiple methods. What happens the next time we have to add another piece of data to that Data Clump? Let’s simplify these signatures by preserving the whole object.
While we’re in refactoring mode, let’s simplify the test.
(Yes, I jumped the gun on the Rule of Three – but it was such obvious duplication, with no additional readability, that it was bugging me.)
We have one more test on our list for this feature. How does the warehouse receive batches of multiple CDs?
To pass this test, we can simply repeat the logic of receiving a single CD in a forEach loop.
Time for one more mini code review before we take a break. It’s important to remember that on any non-trivial design, a perfect score is rarely possible. I’ve made compromises, based on my own judgement and 37 years of programming hard knocks, that I’m happy to live with. Importantly, I thought about them, and I know why I made those compromises (e.g., introducing Feature Envy because I wanted to split up a cohesive class that was doing two distinct jobs – like Catalogue and Warehouse, or the reverse decision to keep searching and adding in Catalogue to avoid un-encapsulating its contents).
As the code currently stands, I’m good to go. No doubt if I leave the design in to soak overnight, it may taste different in the morning.
Let’s cross off the test cases we passed today from our list, and take the night off.
Buy a CD Payment accepted Payment rejected Out of stock
Search for a CD (by title and artist) Warehouse has matching title No match found
Receive batch of CDs Of existing title in catalogue Of new title (add to catalogue) Of multiple titles
- Review a CD
- Just a rating
- Rating and review
Coming next: Reviewing a CD, and a final code review.