Refactoring a legacy code can be a tricky business. More often than not, the code is not tested, unclear and contains some bugs. If not planned, the time required to refactor will eat all the time allocated to the feature. The feature will then be implemented very quickly and it will result in more buggy unmaintainable code.
In this blog post, I will try to show some tools and methods to refactor safely a piece of code. I will use the trivia kata as a support for this. The resulting code can be found on GitHub.
Characterization Test: the Golden Master
The prerequisite to any refactor is to build a test harness to be sure that the refactoring will not break anything. In this example, it could seem like a waste of time to test all possible paths with unit tests. Fortunately, the code contains a lot of traces that print what the code does. It is then possible to run the code with a set of inputs, capture the output and save it. This output is called the Golden Master. Every time the code is modified, the same inputs are used to run the code and the output is compared to the golden master. As you may have noticed, this will not ensure that the code is bug-free, it will just make sure that the code always behaves the same.
In Java, the golden master can be implemented using a library called approval:
<dependency>
<groupId>com.github.nikolavp</groupId>
<artifactId>approval-core</artifactId>
<version>0.3</version>
</dependency>
The associated test is shown below (see commit):
@Test
public void should_record_and_verify_golden_master() {
String result = playGame(1L);
Approvals.verify(result, Paths.get("src", "main", "resources", "approval", "result.txt")); // 2
}
private String playGame(long seed) {
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
System.setOut(new PrintStream(outputStream)); // 3
boolean notAWinner;
Game aGame = new Game();
aGame.add("Chet");
aGame.add("Pat");
aGame.add("Sue");
Random rand = new Random(seed); // 1
do {
aGame.roll(rand.nextInt(5) + 1);
if (rand.nextInt(9) == 7) {
notAWinner = aGame.wrongAnswer();
} else {
notAWinner = aGame.wasCorrectlyAnswered();
}
} while (notAWinner);
return new String(outputStream.toByteArray());
}
Since the code uses a Random to roll a dice, we have to fix the seed of the Random so it always returns the same random rolls in the same order (1). The code has to be run once with this seed and the output has to be pasted into src/main/resources/approval/result.txt to create the golden master (2). Afterwards, the test will capture the output (3) and compare it to the golden master. If a difference appears, a diff will prompt.
Sometimes during my refactoring sessions, I like to make mistakes on purpose just to check the robustness of my test harness. I find it very useful to increase the confidence you have in your tests and fix them if you have to.
First Step: Clean Up
Personally, I can’t really think when I am in front of bloated and unclear code. Thus, the first thing I did to refactor this piece of code is to rearrange it, delete unused imports and dead code, remove magic strings and magic numbers, use generics and reduce the visibility of fields and methods that can be reduced to be sure that I can touch them without breaking the public API. The major part of these refactorings can be made for you by your IDE so don’t hesitate to do it as it reduces the risk of error and it is much quicker. You can also use a plug-in like SonarLint for instance to detect issues directly into your IDE.
Once the code is easier to think about, I noticed that strings were used to identify categories. This seemed like a good place to create an enum (see commit). To start, I just replaced the strings with the enum:
private void askQuestion() {
if (currentCategory() == "Pop")
print(popQuestions.removeFirst());
if (currentCategory() == "Science")
print(scienceQuestions.removeFirst());
if (currentCategory() == "Sports")
print(sportsQuestions.removeFirst());
if (currentCategory() == "Rock")
print(rockQuestions.removeFirst());
}
private String currentCategory() {
if (places[currentPlayer] == 0) return "Pop";
if (places[currentPlayer] == 4) return "Pop";
if (places[currentPlayer] == 8) return "Pop";
if (places[currentPlayer] == 1) return "Science";
if (places[currentPlayer] == 5) return "Science";
if (places[currentPlayer] == 9) return "Science";
if (places[currentPlayer] == 2) return "Sports";
if (places[currentPlayer] == 6) return "Sports";
if (places[currentPlayer] == 10) return "Sports";
return "Rock";
}
became
private void askQuestion() {
if (currentCategory().equals(Category.POP))
print(popQuestions.removeFirst());
if (currentCategory().equals(Category.SCIENCE))
print(scienceQuestions.removeFirst());
if (currentCategory().equals(Category.SPORTS))
print(sportsQuestions.removeFirst());
if (currentCategory().equals(Category.ROCK))
print(rockQuestions.removeFirst());
}
private Category currentCategory() {
if (places[currentPlayer] == 0) return Category.POP;
if (places[currentPlayer] == 4) return Category.POP;
if (places[currentPlayer] == 8) return Category.POP;
if (places[currentPlayer] == 1) return Category.SCIENCE;
if (places[currentPlayer] == 5) return Category.SCIENCE;
if (places[currentPlayer] == 9) return Category.SCIENCE;
if (places[currentPlayer] == 2) return Category.SPORTS;
if (places[currentPlayer] == 6) return Category.SPORTS;
if (places[currentPlayer] == 10) return Category.SPORTS;
return Category.ROCK;
}
This kind of if statement that looks a lot like a switch can almost always be replaced by a Map so that is what I did (see commit). I created two maps to contain questions of each category and the category for each position:
private void askQuestion() {
print(questionsByCategory.get(currentCategory()).removeFirst());
}
private Category currentCategory() {
return categoriesByPosition.get(currentPosition());
}
Second Step: Segregate
The second step I chose to follow is to put specific logic into private methods. Once again, your IDE can help you here, use it!
For instance, that piece of code:
print(players.get(currentPlayer) + " is getting out of the penalty box");
places[currentPlayer] = places[currentPlayer] + roll;
if (places[currentPlayer] >= NB_CELLS) {
places[currentPlayer] = places[currentPlayer] - NB_CELLS;
}
print(players.get(currentPlayer) + "'s new location is " + places[currentPlayer]);
became
print(players.get(currentPlayer) + " is getting out of the penalty box");
move(roll);
print(players.get(currentPlayer) + "'s new location is " + currentPosition());
By doing that, you can remove duplication by factorizing algorithms (like move a player) into specific methods. Moreover, you prepare the field for extracting objects from this code (see the next section). However, in order to do that properly, the private methods have to be in the purest form possible: they should use or modify the least amount of private fields possible (see commit). With that in mind, the following code:
private Category currentCategory() {
return categoriesByPosition.get(currentPosition());
}
has been replaced by
private Category currentCategory(int position) {
return categoriesByPosition.get(position);
}
Third Step: Divide and Conquer
Now that logic has been segregated into private methods, it is pretty straightforward to extract objects from these methods.
In this kata, we saw that several concepts emerge: Player, Board and QuestionDeck. A PlayerList class has also been added to handle the players' rotation. All these classes can be tested in isolation in order not to rely solely on the golden master any more.
For instance, the following code prints the next question to ask:
public void roll(int roll) {
// ...
Category currentCategory = board.categoryOf(newPosition);
print("The category is " + currentCategory);
print(nextQuestionAbout(currentCategory));
}
private String nextQuestionAbout(Category category) {
return questionsByCategory.get(category).removeFirst();
}
By creating a QuestionDeck object, we can have a code like this (see commit):
public void roll(int roll) {
// QuestionDeck deck = new QuestionDeck(NB_QUESTIONS, CATEGORIES);
Category currentCategory = board.categoryOf(newPosition);
print("The category is " + currentCategory);
print(deck.nextQuestionAbout(currentCategory));
}
The private method nextQuestionAbout and the field questionsByCategory are no longer in the Game class.
When you extract classes from the main code, it is very important not to break the public API. It is possible to annotate some methods with @Deprecated though (see commit). A good example of this can be seen in this video.
Fourth Step: Break up
The last step is when we remove all deprecated methods that we introduced during the refactoring. It is not always possible to do it right after the refactoring session as we don’t always have access to the client code that consumes our public API. In this case we do, so I removed these methods (see commit).
The client code looked like this:
Game aGame = new Game();
aGame.add("Chet");
aGame.add("Pat");
aGame.add("Sue");
// aGame.roll()
Now the Game class is not in charge of questions, board, categories and players any more so we have to inject them into the constructor.
private static final int NB_CELLS = 12;
private static final int NB_QUESTIONS = 50;
private static final List<Category> CATEGORIES = asList(Category.POP, Category.SCIENCE, Category.SPORTS, Category.ROCK);
private static final List<String> PLAYERS = asList("Chet", "Pat", "Sue");
// ...
Game aGame = new Game(
System.out,
new Board(NB_CELLS, CATEGORIES),
new QuestionDeck(NB_QUESTIONS, CATEGORIES),
new PlayerList(PLAYERS)
);
// aGame.roll()
Wrapping Up
Of course, as it is always the case with refactoring, I could have done more. The main risk is that more does not always mean better. A refactoring session must have a purpose and it is very important to know when to stop. It could be when you can add your new feature or fix a bug seamlessly or simply because you don’t have any more time allocated to this task.
I hope that this post gave you some tools and methods to refactor your code without the fear of breaking something.
Top comments (0)