Refactoring Legacy Code-Kata : Part 1

Michael Feathers in his book “Working Effectively with Legacy Code” defines legacy code as,

Code without tests is a bad code. It doesn’t matter how well written it is; how well structured it is; how well encapsulated it is.Without tests there is no way to tell if our code is getting better or worse.

What is so wrong with having no tests? I can surely remember the piece of code I wrote yesterday. And I can go way faster and deliver a lot more software if I do not have to write tests.

But… software development is all about solving some problems. Every problem that we solve has a context associated with it. And this context has not been created in a day or two. It has evolved over a period of time. All the lines of code that we write is a translation of that context. Even if you are the only developer working on a program for 10 years, it’s improbable for you to remember all that context. And tests provide this context. Therefore tests are so important for any code.

Change is inevitable. In fact the only thing constant is change. Quite a paradox 😄

And these tests need to be automated. Without automated tests, the only way to determine whether a piece of software is working correctly is to test it manually. Imagine you were working on Apollo 11 with a team of 50 engineers. Would you send Neil Armstrong to the moon every time you introduced a change? Let us say you had all the money and all the fuel in the world to do that. Even then, how can you be sure that on the day of the mission, Apollo 11 can even fly ?

But let’s say you did not write tests.

Maybe you did not think things through. Maybe you just did not have time to invest in tests. Maybe you wanted an MVP to show to to your investors to fund your project. Maybe your boss wanted everything done in a day. Maybe maybe maybe…

The night is darkest just before the dawn. And I promise you, the dawn is coming

Somewhere sometime you have had it. You wanted to add a feature that might have made you rich, but you can’t. The code is rotten. You are petrified to touch that 1000 line method. And you don’t have any tests to validate whether adding that feature will break your code or not. All you do is stare.

So how do you start? Here’s a 4 step algorithm:

• Find out where you are
• Take a small step towards your goal
• Adjust your understanding based on what you learned
• Repeat

When faced with two of more alternatives that deliver
roughly the same value, take the path that makes
future change easier

This above algorithm works almost everywhere. Not just refactoring software. Whenever you want to introduce any change; maybe learn guitar or lose that extra layer of fat; take baby steps. By taking small steps, you try to minimise the fear of introducing that change.

Nuff Chat, Lets Dance

We will take a legacy implementation of a simple game.

Rock Paper Scissors

This is how our code looks today.

We will follow our 4 step algorithm to refactor this problem. I understand that the algorithm seems a bit abstract, but I hope I make things clear when we complete the problem.

We will undergo 5 iterations to refactor the code. With every iteration we will make small improvements to the code. Baby steps. Feel free to stare at the legacy implementation before you jump ahead 😁

Iteration 1

Looking at the code in the first glance, these are the major problems:

1. All the code sits inside a main method

A simple solution would be to move all the code to a public method.

2. Random choices

The program uses (int)(Math.random()*3); to simulate the choices of each player. This does not allow us to write any tests. Let us control this using Dependency Injection. We have added a ChoiceGenerator interface with just 1 implementation RandomChoiceGenerator which is exactly the same as Player.playerChoice. This now allows us to add a FixedChoice implementation for our test

3. The program writes everything to stdout.

Instead of touching the core logic, start returning the stdout. So the void method will now return List<String> which can be easily asserted

4. The Game declares the first player winning 3 games as the winner. This constant value 3 is buried inside the codebase.

Inject the GAME_WON deciding factor. This will enable us to write tests in a more controlled fashion. Also we can default it to 3

5. Values for Rock, Paper and Scissors are not constants.

Move them to an enum

Iteration 1

Iteration 2

Let us now start to add tests assuming the program works. A simple way to write assertions here is actual = expected. This is far from correct, but it’s a starting point.

We will use JUnit5 and AssertJ matchers for our test setup

As we now have a way to inject FixedChoiceGenerator and GameWonCriteria, out first test is with Player1 playing Rock and Player2 playing Paper. As we know Paper covers Rock, so Player2 wins

We create a stub for the ChoiceGenerator and complete our first awesome test.

While running it, this happens

***** Round: 0 *********************
Number of Draws: 0
Player 1: rock	 Player 1 Total Wins: 0
Player 2: paper	 Player 2 Total Wins: 0
Player 2 Wins
***** Round: 1 *********************
Number of Draws: 0
java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2

Now we wanted the program to stop after 1 iteration, and our choices seemed to have been stubbed properly. Then what has gone wrong?

Just look at the following lines of code:

int p1Wins = p1.wins;
  ..
  ..
     p2Wins++;  // trying a couple different ways to get count to work
  ..
  ..
   if ((p1.getWins() >= gamesToWin) || (p2.getWins() >= gamesToWin)) {
        gameWon = true;

We are assigning p1.wins to a variable, incrementing it and then using getWins getter to fetch its value. The player class does have a setter to update win count. There is also an amazing comment which points us to the same direction.

We can fix this by changing p2Wins++ to p2.setWins(). Just like that, we fixed a small bug and have a test to assert it... 🤘

Looking at the test output, a couple of things still seem to be off:

• The Round starts with zero
• Player 2 Total wins is still 0.

Going back to the code, we understand that this summary is printed before the actual evaluation of choices.

// Game Loop    
do {      
System.out.println("***** Round: " +          roundsPlayed + " *********************\n");      
System.out.println("Number of Draws: " +          draw + "\n");      p1Choice = p1.playerChoice();      
System.out.println("Player 1: " + p1Choice +          "\t Player 1 Total Wins: " + p1Wins);      p2Choice = p2.playerChoice();      System.out.println("Player 2: " + p2Choice +          "\t Player 2 Total Wins: " + p2Wins);

This is another bug that needs attention. But since we do not have enough test coverage, we will come back to this later.

Iteration 2

Now, we are getting somewhere 🤘. Let’s break for some coffee before we move on. After our coffee break, we will continue on to the remaining 3 iterations.