Refactoring Legacy Code In Practice – Iteration 1

If you have been comdamned to work on old, ugly, unmantained and untested code, then you probably feel the urge from time to time to clean up a little bit and make your life easier. Let’s face it, cleaning up some mess in the code is always fun, until it’s not: until you have to work in the night to fix some bug you introduced in a core feature just because you wanted to clean up the code.

You know there’s a huge risk in every change you make, but you want to work with better code, so you have two options: you either clean it or you leave your job! Leaving a job is slow and painful and you have no guaranties that you are going to land a job which offers better code to work on, so your only choice is to clean up.

But, where to start? How can you minimize the risks? well, we have written a lot about how to handle legacy code (have a look at our Blog series page) and there are some great books that you can read (go to our Resources page). But what if you want to practice and experiment in a safe environment without the risk of breaking your production code? With some luck you can find some User Group near to you that hosts a Legacy Code Retreat but if you are nowhere near a developers meetup like I am you can always find some ugly source code and try to practice by yourself the same excercices they do in the code retreats. And what better code can you use for this purpose than  one that was specifically designed to be legacy: just go here, download it and start refactoring.

That’s exactly what I did and I wanted to share the process with you in this series of post so you can compare your approach to that of a fellow programmer.

First iteration

Like every legacy project that’s worth its name, there is not enough information about the code you are going to touch. So the first iteration is about discovering how bad its state is and what’t the whole purpose of the code. If you had time you could try to apply some refactoring techniques trying to minimize the risk of adding bugs. After 45 minutes you had to stop and delete everything you have done so far.

After some analysis I discovered that this game is basically a simplified implementation of the Trivial Pursuit. It’s formed by two classes the Game class that has all the business logic of the game and the GameRunner that it’s basically the main entry point of the program. For some obscure reason the game loop is located in the GameRunner.

After starting the game, the game runner adds three players to the game and then starts the game loop that can be described by these steps:

1. a player rolls a standard 6 faces dice
2. based on the result and the player’s current status the player moves or remains in the same spot
3. A question is asked to the player which responds to the question with 1/9 percent change of getting it wrong
4. the game continues until there is a winner: the first one to reach 6 points.

I still haven’t been able to understand how does the player gains it’s points.

The code

Exploring the code gives you headache: you cannot understand if it was so badly crafted written for the purpose of the exercises or if JB asked a fifteen years old kid who is trying to learn to program to write it. I have tried to build examples of bad code and was never able to create something so beautifully ugly!

By the way let’s have a look at the code:

The game runner starts instantiating a Game object and then it calls and add method with something that looks like player names.

Game aGame = new Game();

aGame.add("Chet");
aGame.add("Pat");
aGame.add("Sue");

The method add in fact adds the player name to a list of strings called players then, it looks like it places the player in the first box with zero points, then it returns true (why returning true? what does it means?).

public bool add(String playerName)
{
    players.Add(playerName);
    places[howManyPlayers()] = 0;
    purses[howManyPlayers()] = 0;
    inPenaltyBox[howManyPlayers()] = false;

    Console.WriteLine(playerName + " was added");
    Console.WriteLine("They are player number " + players.Count);
    return true;
}

Back to the GameRunner class. In the game loop a random number between 1 and 6 is picked and then this number is passed to the roll method. Here there is some duplication and some business logic difficult understand, for example: if inPenaltyBox for the current player is true and the random number (roll) is not even (why on earth check that something is not even? wasn’t it the same checking that it was odd?) a variable called isGettingOutOfPenaltyBox becomes true.

if (inPenaltyBox[currentPlayer])
{
    if (roll % 2 != 0)
    {
        isGettingOutOfPenaltyBox = true;

        Console.WriteLine(players[currentPlayer] + " is getting out of the penalty box");
        places[currentPlayer] = places[currentPlayer] + roll;
        if (places[currentPlayer] > 11) places[currentPlayer] = places[currentPlayer] - 12;

        Console.WriteLine(players[currentPlayer]
                + "'s new location is "
                + places[currentPlayer]);
        Console.WriteLine("The category is " + currentCategory());
        askQuestion();
    }
    else
    {
        Console.WriteLine(players[currentPlayer] + " is not getting out of the penalty box");
        isGettingOutOfPenaltyBox = false;
    }

The askQuestion method checks which category is the player in and pops a question from the question from the queue (smells like bug… what happens if you run out of questions of a certain category? a you get a nice little exception!).

if (currentCategory() == "Pop")
{
    Console.WriteLine(popQuestions.First());
    popQuestions.RemoveFirst();
}

The currentCategory method looks like a really malformed array of strings, but you cannot be sure until you know know how many places there are in the game and which one are actually of “Rock” category, are there some bonus places not considered in this method?

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";
}

 Refactoring

The first thing I would like to address is the players management, there should be a Player class that handles most of the actions on the player, like it’s points or if it’s in a penalty round. Another improvement could be to isolate is the Board itself in a class that knows places and categories for each one. If you then extract a set of classes that represent the deck of cards and move the game loop inside the game class.

The problem is that these entities are spread across multiple methods and extracting requires changing a lot of places, so it would be better to first have a comprehensive suite of automated tests.

Simple things first

There’s not enough time left so I look for the simplest method to start doing some real refactoring.  I don’t really like currentCategory, there’s a lot of  duplicated code, and a ton of ifs.

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";
}

The first thing to do is to build the test fixture and to do so I apply subclass to test and promote the method from “private” to “protected virtual” to be able to access it from a child class.

protected string CurrentCategory()

Then I can subclass the Game class and expose the protected method with a public one so I can call it in my tests and the I can start building the test suite.

private class GameForTest : UglyTrivia.Game
{
    public string PublicCurrentCategory()
    {
        return CurrentCategory();
    }

    public int Place
    {
        set { Places[0] = value; }
    }
}

Using subclass to test in often considered an anti-pattern for various reasons one been that with it you test the internal of a class instead of shifting to a higher level of abstraction but in this context is permitted because we just want to preserve the functionality while changing the code and we don’t care if we have to get rid of the test later on.

Building the tests

After having exposed CurrentCategory as a public method we can stub also the places array in order to have fine control over which place a given player is in (in our case player 0). In our case this gives us some advantages like not having roll the dice to move the player arround, just set it’s position do the desired value:

[TestCase(0)]
[TestCase(4)]
[TestCase(8)]
public void CategoryPop(int place)
{
    // we need to set differnt values for places[currentPlayer]
    var game = new GameForTest {Place = place};

    var currentCategory = game.PublicCurrentCategory();
    // I expect it to be

    Assert.AreEqual("Pop", currentCategory);
}

[TestCase(1)]
[TestCase(5)]
[TestCase(9)]
public void CategoryScience(int place)
{
    var game = new GameForTest {Place = place};
    var currentCategory = game.PublicCurrentCategory();
    Assert.AreEqual("Science", currentCategory);
}

[TestCase(2)]
[TestCase(6)]
[TestCase(10)]
public void CategorySports(int place)
{
    var game = new GameForTest {Place = place};
    var currentCategory = game.PublicCurrentCategory();
    Assert.AreEqual("Sports", currentCategory);
}

[TestCase(3)]
[TestCase(7)]
[TestCase(11)]
public void CategoryRock(int place)
{
    var game = new GameForTest {Place = place};
    var currentCategory = game.PublicCurrentCategory();
    Assert.AreEqual("Rock", currentCategory);
}

I wrote four simple parametrised tests, one for each category, and the test’s parameter is the place of which I want to check it’s category. As you can see the tests assert that there are 12 positions and that positions 3, 7 and 11 are Rock category: this is an assumption based on what I’ve learned inspecting the code.

Ok, now that the method is covered I’m free to try different things. First of all I substituted the ifs with a much simpler switch statement, but the conditionals are still there. The tests pass. Then I replaced the switch with an array of strings and promoted it to become a private field of the Game class. The tests still pass. With one more step I convert the array of strings in an array of enum because I don’t like magic numbers; The method still have to return a string because if i change the return I’ll have to make changes in a lot of different places in the code. Again, the tests still pass.

private readonly Category[] _categories = {
        Category.Pop,
        Category.Science,
        Category.Sports,
        Category.Rock,
        Category.Pop,
        Category.Science,
        Category.Sports,
        Category.Rock,
        Category.Pop,
        Category.Science,
        Category.Sports,
        Category.Rock
    };

protected string CurrentCategory()
{
    var place = Places[_currentPlayer];
    return _categories[place].ToString();
}

End of the iteration

I ran out of time after this refactoring, but there is no doubt that my understanding of the code has increased a lot and that the little method I chose to refactor is a lot easier to understand now that there are no conditionals.

If you want to practice your refactoring muscles download the project at JB’s repository, or you can download it form mine where I divided every iteration in a different branch so you can explore what I did.

In the next iteration I’m going characterize the behavior of the Game class building a Golden Master, subscribe if you don’t want to miss it!