Agile Legacy Code

Primary Menu

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.

privateclassCardDeckForTests : 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.

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.

protectedvoidaskQuestion()

{

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:

protectedoverridestringcurrentCategory()

{

return CategoryStub;

}

public stringCategoryStub { 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

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.

protectedvoidaskQuestion()

{

stringquestion = 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;

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.

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.

protectedvoidaskQuestion()

{

varcategory = currentCategory();

varquestion = _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:

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]

publicvoidGameAsksQuestionForCategory()

{

varstream = newMemoryStream();

varwriter = newStreamWriter(stream);

varreader = newStreamReader(stream);

Console.SetOut(writer);

vargame = newGameForTests();

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.

publicclassGame

{

private readonly CardDeck_deck;

publicGame(CardDeck deck)

{

_deck = deck;

}

// ...

protectedvoidaskQuestion()

{

varcategory = currentCategory();

varquestion = _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.