Refactoring the Video Store Kata — Part 1

I have recently been trying to improve my refactoring ability using the Video Store.

I have only attempted one other refactoring kata, Gilded Rose, but I think both katas left me with the same sense of confusion when I first looked over them. This blog post will detail the initial steps I took to refactor the code:

The first few things I did were quick wins such aligning and indenting the code properly so that I could understand it better.

The tests in the test class were laid out in a very confusing way.

Mainly the assertion was clumped into one big string:

"Rental Record for Fred\n\tThe Cell\t9.0\nYou owed 9.0\nYou earned 2 frequent renter points\n"

Not only did this mean you had to scroll right to read the whole line but also it was difficult to pick out what was important in the code. To make the overall intention of the tests clearer, I split this string by each line break.

"Rental Record for Fred\n" +
"\tThe Cell\t9.0\n" +
"You owed 9.0\n" +
"You earned 2 frequent renter points\n"

The main aim of the statement was to inform the customer what they owed and what their renter points were. I extracted these two values and deleted the rest of the statement string template. Once I had applied this refactoring to the rest of the tests they were much easier to digest and also gave me more of a focus for refactoring the rest of the code.

The overall string still needed to be tested at least once so I kept one test which tested the string in its entirety.

public String statement() {
  double totalAmount = 0;
  int frequentRenterPoints = 0;
  Enumeration rentals = this.rentals.elements();
  String result = "Rental Record for " + getName() + "\n";

  while (rentals.hasMoreElements()) {
    double thisAmount = 0;
    Rental each = (Rental) rentals.nextElement();

    switch (each.getMovie().getPriceCode()) {
      case Movie.REGULAR:
        thisAmount += 2;
        if (each.getDaysRented() > 2)
          thisAmount += (each.getDaysRented() - 2) * 1.5;
        break
      case Movie.NEW_RELEASE
        thisAmount += each.getDaysRented() * 3
        break;
      case Movie.CHILDRENS:
        thisAmount += 1.5;
        if (each.getDaysRented() > 3)
          thisAmount += (each.getDaysRented() - 3) * 1.5;
        break;
  }
  frequentRenterPoints++;
  if (each.getMovie().getPriceCode() == Movie.NEW_RELEASE &&   each.getDaysRented() > 1)
    frequentRenterPoints++;
  result += "\t" + each.getMovie().getTitle() + "\t" + String.valueOf(thisAmount) + "\n";
  totalAmount += thisAmount;
 }

  result += "You owed " + String.valueOf(totalAmount) + "\n";
  result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points\n";

  return result;
}

In the Customer class, the values printed in the final string that made up the actual statement were local variables in the statement() method. There variables were named getFrequentRenterPoints and thisAmount. I converted these two variables into instance variables and created the methods getFrequentRenterPoints() and getAmountOwed() which returned these instance variables which meant I could test if these values were correct.

The last few refactorings I did in the tests were to remove import JUnit. This meant I could remove the line “extends TestCase” which is based on an old testing convention.

I then removed the duplicated strings for each different movie name and type by extracting the strings into fields.

I named the extracted fields variables after their movie types so a New Release type became newRelease1 and then any other movies with different names but the same movie type were called newRelease2 and so on.

I also inline the frequentRenterPoints and amountOwed variables to make the test most concise.

public class VideoStoreTest {
private final Movie newRelease1 = new Movie("The Cell", Movie.NEW_RELEASE);
private Customer customer;
BEFORE
@Test
  public void testSingleNewReleaseStatement() {
  customer.addRental(new Rental(new Movie("The Cell", Movie.NEW_RELEASE), 3));
  double amountOwed = 9.0;
  int frequentRenterPoints = 2;
  customer.statement();
  assertEquals(amountOwed, customer.getAmountOwed());
  assertEquals(frequentRenterPoints,
customer.getFrequentRenterPoints());
}

AFTER
@Test 
  public void testSingleNewReleaseStatement() {
  customer.addRental(new Rental(newRelease1, 3));
  customer.statement();
  assertEquals(9.0, customer.getAmountOwed());
  assertEquals(2, customer.getFrequentRenterPoints());
}

I then looked at the huge statement() method on the Customer class and tried to think of way that I could cut it down.

I knew there were two key values that I was looking to focus on — frequentRenterPoints and totalAmount — and they were both heavily tangled up in the statement() method code. By looking at the numerous lines of code and different elements involved in producing the overall statement, it was clear that this method was doing too much.

I decided to use the extract method refactoring to isolate some of these key elements.

The first step was to extract the logic for getting the renter points to its own method:

private void getFrequentRenterPoints(Rental eachMovie) {
  frequentRenterPoints++;
  if (eachMovie.getMovie().getPriceCode() == Movie.NEW_RELEASE 
  && eachMovie.getDaysRented() > 1)
  frequentRenterPoints++;
}

This cut down my statement() method so it seemed like a helpful refactoring. However, incrementing the instance variable I had made earlier meant I was creating mutable state. My code had become fragile.

To remove this mutable state I looked at what this method was actually doing:

1. It was increasing the frequentRenterPoints by one for each movie.
2. If the movie was a new release and had been rented for more than one day then another renter point was added to the overall points.

This method was unnecessarily confusing. To make it simpler, I converted the frequentRenterPoints instance variable to a local variable named points then I added either 1 or 2 to the points depending on whether the movie met the condition.

private int frequentRenterPoints(Rental rental) {
  int points = 0;
  if (rental.getMovie().getPriceCode() == Movie.NEW_RELEASE && rental.getDaysRented() > 1) {
    points += 2;
  } else {
    points += 1;
  }
  return points; 
}

I then realised I didn’t actually need the points variable as I was just adding 1 or 2 to 0. Therefore, I removed it and made the method even simpler.

private int frequentRenterPoints(Rental rental) {
    if (rental.getMovie().getPriceCode() == Movie.NEW_RELEASE && rental.getDaysRented() > 1) {
        return 2;
    } else {
        return 1;
    }
}

I then returned the points and added them to the frequentRenterPoints instance variable in the main statement method. I had removed the state from this method but the idea was to remove in completely from my program.

In my next blog post I will discuss:

• How I refactored the totalAmount variable.
• How to remove all of the mutable state from my program.
• How I simplified the statement() method further by extracting the case statement into different classes and using polymorphism.