Can't sleep at night? Feeling a bit unkempt, like something was left in disarray? Me, too. Last month we walked away from our code knowing that
it was a mess. (Hey, they only give me so much space here.) But we won't let another sleepless night pass until we've cleaned up the clutter. So break out your favorite coding tools: We've got work to do!
Get Green Early
In the rush to make our tests pass, we committed a cardinal programming sin: We created duplication. If
we need to make a change, we now have to make it in two places. It all started with a playTime() method that calculates how long you can listen to a playlist without repeating a song (1).
The code is clean and well documented, and we even covered it with a unit test to make sure the code doesn't break without our knowing. Indeed, it's good code.
Then, when it came time to write a listenTime() method that calculates how much time you've spent listening to the playlist, we just copied the playTime() method and made one minor change to the
for loop (2):
All the tests are passing now, but we don't feel good about the gross duplication in the playTime() and listenTime() methods. Why did we do it? Well, at the time we were focused on making the code
work right. Had we tried to generalize the code for both methods too early, we might have broken what little functionality we already had working. But now that
we have tests that cover both methods, we can confidently shift our focus from making the code work to cleaning it up so that it's responsive to change.
You see, creating duplicated code isn't necessarily bad; it's leaving duplicated code to fester that creates maintenance nightmares. And without good tests, cleaning up clutter is risky—and even downright scary. Consequently, bad code just keeps getting worse.
Extraction: Just What the Doctor Ordered
We have almost the same code in two methods of the
Playlist class. Let's start to consolidate the code by refactoring at the source of the duplication—the playTime() method—to see what falls out. The first thing we see is a loop (3).
The presence of that comment is a pretty good indication that the readability of the code that follows it could be improved. Instead of using a comment, let's turn the code block into a well-named method (4).
That's a fairly straightforward refactoring: Move the code into a method that returns the value of the calculation. But having extracted methods manually a few times making silly mistakes along the way, I've come to
appreciate what an IDE that supports automated refactoring can do for me. (Don't worry, I still use vi and
emacs when appropriate.)
If we're using Eclipse, for example, we just highlight the code block, select Refactor > Extract
Method, and specify the new method name. Eclipse then creates the new method, copies the highlighted code block into the new method, and replaces the highlighted code with a call to the extracted method. Now that might not sound
like much, but Extract Method is a fundamental refactoring that you'll use time and again to clean up
code. And if you're like me, when something is time consuming or error prone, then I don't want to do it very often. Using a tool that automates refactorings makes it efficient and safe to keep the code clean and the tests green.
Don't Listen to the Locals
The next thing we see in the playTime() method is three local variables holding the results of time calculations (5).
These local variables are assigned to once, and then they are used immediately afterward in the final section of code. This prompts a question: What purpose do the variables serve? Well, they give a meaningful name
to the result of their corresponding calculation. For example, seeing the assignment to the hours variable tells us that the expression to its right results in a number expressed in hours. (We hope.) Then whenever we see that variable again in the method, we know that it represents hours.
But that useful information only lives until the end of the method when the variables go out of scope. And we know that the listenTime() method performs these
same calculations, so let's extract a few more well-named methods that we can use elsewhere in the class (6).
(To do this in Eclipse, highlight the original line and use Refactor > Extract Method.)
At this point, the variable names don't give us any more
information than we can glean from the method names. In fact, it's duplicated knowledge that should be eliminated, which brings us to the next section of code (7).
Here we see the one and only use of the local variables. Hmm . . . what do they buy us? Now that we have well-named methods that return these values, we can get rid of the local variables altogether. We just replace the references to each variable with calls to the appropriate method
(8).
(To do this in Eclipse, highlight the reference to the local variable and use Refactor > Inline.)
We're almost done, but let's not stop short. We'll need this one-line string concatenation in the listenTime() method. And we're getting pretty good at extracting methods, so let's put the formatting of the time string in a method of its own (9).
Phew! After that flurry of coding, we've whittled the
playTime() method down to almost nothing
(10).
And to make it a bit more readable, we can take it one step further (11).
It's been a while since we've run the test. Did we break
anything? We won't know until we run the tests. Thankfully they pass, but if they didn't, we could backtrack until they passed again. Refactoring is about making code easier to modify, with no observable change in behavior. The tests
are there to ensure that behavior remains unchanged.
Cleaning Up the Mess
All that work, and we still haven't cleaned up the duplication in the listenTime() method. Ah, but
by extracting methods from the playTime() method, our work is made easier. The only differentiator in the listenTime() method is how the total
seconds are calculated (12).
You've been paying attention if you're thinking this code deserves to be extracted into a well-named method of its own, perhaps called listenTimeInSeconds().
The rest of the listenTime() method is code we already have in methods of the Playlist class, so we can reduce listenTime() down, and
voila! (13).
The top layer of duplication is gone, the tests still pass, and we'll sleep soundly tonight. We made a lot of progress quickly using safe, effective refactorings. But in our wake we left several small methods: toSeconds(), toMinutes(), toHours(), and formattedTime(). It turns out those methods have a secret to tell us. We'll save that for the next issue.
