Refactoring Legacy Code In Practice – Iteration 3

After building the Golden Master in the previous iteration we are sure that we wont introduce any bugs during our refactoring phase: we built a safety net that will prevent us form falling into the regression trap.

The next refactoring I’m going to practice is Subclass to test followed by replace inheritance with delegation. As you can see here subclass to test is a powerful technique to break dependencies and test only the method we are interested in refactoring , but it’s also considered by some an anti-pattern because the complexity of the code becomes higher and higher making it difficult to apply refactoring techniques later on.

Testing the questions’ initialization

In this exercise I decided to refactor the askQuestion method of the Game class.

The first step in building the test suite is to create a class that inherits from the Game class in order to be able to reach it’s internals without actually making them public.

private class CardDeckForTests : CardDeck
{

To test the askQuestions I can tests that it pops the correct questions from the correct category and to make my life a little easier I decided to spy on the values contained by the questions’ linked lists, so the next step was to promote the linked lists to protected and expose them in the test class.

private class CardDeckForTests : CardDeck
{
public LinkedList<string> PopQuestions { get { return popQuestions; } }
public LinkedList<string> RockQuestions { get { return rockQuestions; } }
public LinkedList<string> ScienceQuestions { get { return scienceQuestions; } }
public LinkedList<string> SportsQuestions { get { return sportsQuestions; } }

public class Game
{
// ...
protected LinkedList<string> popQuestions = new LinkedList<string>();
protected LinkedList<string> scienceQuestions = new LinkedList<string>();
protected LinkedList<string> sportsQuestions = new LinkedList<string>();
protected LinkedList<string> rockQuestions = new LinkedList<string>();
// ...

And then my first test checks that the questions get correctly initialized in the constructor

[Test]
public void BoardInizializzedCorrectly()
{
var game = new GameForTests();
Assert.AreEqual(50, game.PopQuestions.Count);
Assert.AreEqual(50, game.RockQuestions.Count);
Assert.AreEqual(50, game.ScienceQuestions.Count);
Assert.AreEqual(50, game.SportsQuestions.Count);
for (int i = 0; i < 50; i++)
{
Assert.AreEqual("Pop Question " + i, game.PopQuestions.First());
Assert.AreEqual("Rock Question " + i, game.RockQuestions.First());
Assert.AreEqual("Science Question " + i, game.ScienceQuestions.First());
Assert.AreEqual("Sports Question " + i, game.SportsQuestions.First());
game.PopQuestions.RemoveFirst();
game.RockQuestions.RemoveFirst();
game.ScienceQuestions.RemoveFirst();
game.SportsQuestions.RemoveFirst();
}
}

Why should I care?

To test the askQuestion method it would have been enough to stub out the lists instead of checking that they get correctly initialized.  I’m doing this extra work because I’m aiming at extracting a class that represents the deck of cards and testing that the lists get initialized correctly will help me testing that the deck class gets correctly initialized too.

Second step: test askQuestion’s behaviour

Looking closer at the method askQuestion I notice that it have  one indirect input in the form of a method call to currentCategory, one indirect output as it writes one question to the console and some side effects when it pops the questions from the linked lists.

protected void askQuestion()
{
if (currentCategory() == "Pop")
{
Console.WriteLine(popQuestions.First());
popQuestions.RemoveFirst();
}
if (currentCategory() == "Science")
{
Console.WriteLine(scienceQuestions.First());
scienceQuestions.RemoveFirst();
}
if (currentCategory() == "Sports")
{
Console.WriteLine(sportsQuestions.First());
sportsQuestions.RemoveFirst();
}
if (currentCategory() == "Rock")
{
Console.WriteLine(rockQuestions.First());
rockQuestions.RemoveFirst();
}
}

My goal with the test is to check that for any possible indirect input, the indirect output and the side effect are what I expect them to be: for example if currentCategory returns “Rock”, then the method should write the next rockQuestion to  the console and then pop it out of the list.

To do so I want to control the values returned by currentCategory so, first of all I add a public property to GameForTests that I can set during my tests with the value I want currentCategory to return, then I promote the later to protected virtual in order to override it and make it return the value of the new field:

protected override string currentCategory()
{
return CategoryStub;
}
public string CategoryStub { get; set; }

Then I build a test that asks one question of each category checking that the question gets popped from the linked list and that it’s sent to the console

[Test]
public void DeckReturnsQuestionForCategory()
{
// change the output stream of console to test
// the indirect output
var stream = new MemoryStream();
var writer = new StreamWriter(stream);
var reader = new StreamReader(stream);
Console.SetOut(writer);
// Initialize the game
var game = new GameForTests();
// stub the category and ask the question
game.CategoryStub = "Rock";
game.AskQuestion();
// check that the first question was popped out of the list
Assert.AreEqual(49, game.RockQuestions.Count);
Assert.IsFalse(game.RockQuestions.Any(x => x.Equals("Rock Question 0")));
Assert.AreEqual(50, game.PopQuestions.Count);
Assert.AreEqual(50, game.ScienceQuestions.Count);
Assert.AreEqual(50, game.SportsQuestions.Count);
// same thing for other categories
writer.Flush();
stream.Position = 0;
// Check that the indirect output is correct
Assert.AreEqual("Rock Question 0", reader.ReadLine());
stream.Close();
}

Preparing to extract the class

Now that we have the method tested through “subclass to test”, we want to extract the tested code as a dependency in order to separate the miss-placed behavior. the next move then is to prepare the code to extract a class clearly isolating the parts that could go in the extracted class from those that need to remain.

I start by moving the `Console.WriteLine` calls out of the condition blocks so that the askQuestion first evaluate a temporary string that then gets passed to the indirect output.

protected void askQuestion()
{
string question = string.Empty;
if (currentCategory() == "Pop")
{
question = popQuestions.First();
popQuestions.RemoveFirst();
}
if (currentCategory() == "Science")
{
question = scienceQuestions.First();
scienceQuestions.RemoveFirst();
}
if (currentCategory() == "Sports")
{
question = sportsQuestions.First();
sportsQuestions.RemoveFirst();
}
if (currentCategory() == "Rock")
{
question = rockQuestions.First();
rockQuestions.RemoveFirst();
}
Console.WriteLine(question);
}

Then I change the questions fields to readonly properties that expose a private fields;

public class Game
{
protected LinkedList<string> popQuestions { get { return _popQuestions; } }
protected LinkedList<string> scienceQuestions { get { return _scienceQuestions; } }
protected LinkedList<string> sportsQuestions { get { return _sportsQuestions; } }
protected LinkedList<string> rockQuestions { get { return _rockQuestions; } }
private readonly LinkedList<string> _popQuestions = new LinkedList<string>();
private readonly LinkedList<string> _scienceQuestions = new LinkedList<string>();
private readonly LinkedList<string> _sportsQuestions = new LinkedList<string>();
private readonly LinkedList<string> _rockQuestions = new LinkedList<string>();

And the body of the constructor becomes a call to a method that initializes the deck.

All my tests still pass so I’m good to go;

Extract the Parent class

I then change the Game class so it inherits form a parent class called, you guessed it, GameParent. The purpose of this move is to move all the logic for asking a question out of the game class so it will depend only on a call to a parent method. I still don’t need to change my test.

public class GameParent
{
protected LinkedList<string> popQuestions { get { return _popQuestions; } }
protected LinkedList<string> scienceQuestions { get { return _scienceQuestions; } }
protected LinkedList<string> sportsQuestions { get { return _sportsQuestions; } }
protected LinkedList<string> rockQuestions { get { return _rockQuestions; } }
private readonly LinkedList<string> _popQuestions = new LinkedList<string>();
private readonly LinkedList<string> _scienceQuestions = new LinkedList<string>();
private readonly LinkedList<string> _sportsQuestions = new LinkedList<string>();
private readonly LinkedList<string> _rockQuestions = new LinkedList<string>();
public GameParent()
{
InitDeck();
}
private void InitDeck()
{
for (int i = 0; i < 50; i++)
{
_popQuestions.AddLast("Pop Question " + i);
_scienceQuestions.AddLast(("Science Question " + i));
_sportsQuestions.AddLast(("Sports Question " + i));
_rockQuestions.AddLast(createRockQuestion(i));
}
}
public String createRockQuestion(int index)
{
return "Rock Question " + index;
}
protected string Question(string category)
{
string question = string.Empty;
if (category == "Pop")
{
question = popQuestions.First();
popQuestions.RemoveFirst();
}
if (category == "Science")
{
question = scienceQuestions.First();
scienceQuestions.RemoveFirst();
}
if (category == "Sports")
{
question = sportsQuestions.First();
sportsQuestions.RemoveFirst();
}
if (category == "Rock")
{
question = rockQuestions.First();
rockQuestions.RemoveFirst();
}
return question;
}
}

Yon may notice that it has a protected method called Question that gets a category and returns the question to be asked: This method doesn’t have any indirect inputs nor it has indirect outputs, those remain in the askQuestion method which is now super simple: it retrieves the category, then gets the question form the parent and then sends it to the console.

protected void askQuestion()
{
var category = currentCategory();
var question = _deck.Question(category);
Console.WriteLine(question);
}

Extract the dependency

Now that the code for building a deck and retrieving questions form it is isolated inside the parent class we can apply the Replace inheritance with delegation refactoring to break the parent-child dependency.

Making this change requires to first change some tests in order to have them pointing directly to the parent class. This isn’t a breaking change but the Game class looses it’s coverage so I’m also forced to write a new test to be sure that the behavior of the askQuestion method doesn’t change.

I first rename the parent from GameParent to CardDeck and build a child class to test:

private class CardDeckForTests : CardDeck
{
public LinkedList<string> PopQuestions { get { return popQuestions; } }
public LinkedList<string> RockQuestions { get { return rockQuestions; } }
public LinkedList<string> ScienceQuestions { get { return scienceQuestions; } }
public LinkedList<string> SportsQuestions { get { return sportsQuestions; } }
public string AskQuestion(string category)
{
return Question(category);
}
}

then change the test methods to test the new CardDeckForTests.

[Test]
public void DeckInizializzedCorrectly()
{
var deck = new CardDeckForTests();
Assert.AreEqual(50, deck.PopQuestions.Count);
Assert.AreEqual(50, deck.RockQuestions.Count);
Assert.AreEqual(50, deck.ScienceQuestions.Count);
Assert.AreEqual(50, deck.SportsQuestions.Count);
for (int i = 0; i < 50; i++)
{
Assert.AreEqual("Pop Question " + i, deck.PopQuestions.First());
Assert.AreEqual("Rock Question " + i, deck.RockQuestions.First());
Assert.AreEqual("Science Question " + i, deck.ScienceQuestions.First());
Assert.AreEqual("Sports Question " + i, deck.SportsQuestions.First());
deck.PopQuestions.RemoveFirst();
deck.RockQuestions.RemoveFirst();
deck.ScienceQuestions.RemoveFirst();
deck.SportsQuestions.RemoveFirst();
}
}
[Test]
public void DeckReturnsQuestionForCategory()
{
var deck = new CardDeckForTests();
Assert.AreEqual("Rock Question 0", deck.AskQuestion("Rock"));
Assert.AreEqual(50, deck.PopQuestions.Count);
Assert.AreEqual(49, deck.RockQuestions.Count);
Assert.AreEqual(50, deck.ScienceQuestions.Count);
Assert.AreEqual(50, deck.SportsQuestions.Count);
// Other Categories
}

As you can see this test doesn’t need to redefine the Console’s output stream as the Question method returns a string and doesn’t have indirect outputs. The new test that I have to write checks that, for any given category, the askQuestion in the Game class sends the question to the console like before:

[Test]
public void GameAsksQuestionForCategory()
{
var stream = new MemoryStream();
var writer = new StreamWriter(stream);
var reader = new StreamReader(stream);
Console.SetOut(writer);
var game = new GameForTests();
game.CategoryStub = "Rock";
game.AskQuestion();
game.CategoryStub = "Pop";
game.AskQuestion();
game.CategoryStub = "Science";
game.AskQuestion();
game.CategoryStub = "Sports";
game.AskQuestion();
writer.Flush();
stream.Position = 0;
Assert.AreEqual("Rock Question 0", reader.ReadLine());
Assert.AreEqual("Pop Question 0", reader.ReadLine());
Assert.AreEqual("Science Question 0", reader.ReadLine());
Assert.AreEqual("Sports Question 0", reader.ReadLine());
stream.Close();
}

now that I’m happy with the status of my tests I’m ready to break the inheritance. Now the game class doesn’t inherits from anyone and it has a CardDeck as a private field that gets assigned in the constructor and the ask Question simply calls the Question method of the CardDeck field and it has no clue of how the questions are stored internally in the CardDeck class.

public class Game
{
private readonly CardDeck _deck;
public Game(CardDeck deck)
{
_deck = deck;
}
// ...
protected void askQuestion()
{
var category = currentCategory();
var question = _deck.Question(category);
Console.WriteLine(question);
}
// ...
}

Conclusion

Even if a little bit longer than just asking to ReSharper to extract a class, proceeding with this refactoring gave me the opportunity to make every step in a save and clean way without suddenly breaking the test and having to fix them later. The only time the test got broken was when I needed to change the structure of the code so heavily that I decided to change the test to reflect the behavior that I’m expecting from the change.

What struck me was that at every change it was crystal clear to me what was the next step to be taken in order to improve the code. This doesn’t happen a lot when refactoring code at work when you have less time and have to take shortcuts.

If you liked this article and want more Subscribe here and you wont miss any article.

Author: Daniele Pozzobon

Daniele is an aspiring software craftsman with more that ten years of experience in the software industry. He is currently a consultant in the .Net space for a big insurance company, and previously have worked as a web dev in the manufacturing industry. He has experience with C#, Java, C++, PHP, Javascript, and lately has added some F# to the sauce.
He constantly annoys his friends by talking about software and is passionate about Agile methodologies, which gives him more opportunities to talk annoy his friends even more.
When there are no friends around to annoy, he blogs on CodeCleaners and in his free he time loves go hiking with his wife and two daughters.