This post is the step following the beginning of the refactoring exercise on the command line video store code base (Java, C#).
In the previously described refactoring step, we removed a primitive obsession smell and introduced a new class Movie.
The code for the beginning of this exercise can be found on my repo on branch E2. The end of this refactoring step is located here.
This time, we will continue beating that unwieldy piece of procedural code into submission by further extracting objects out of it.
Making implicit concepts explicit
If we look at the current state, we may notice that the code contains an implicit concept.
In the big while loop, we “determine amounts for the rental”. But what exactly is done there? Rental data is read from the user’s input: the movie and the number of days it was rented for. Based on this, the code calculates frequent renter point and the amount that will be billed to the customer.
There is an opportunity to make the concept of Rental explicit: a Rental is comprised of a movie and a number of days, and we can calculate the renter points and the billed amount based on this data.
This just what we need to make the code object-oriented: data and behaviour. Let’s introduce a new domain class named Rental.
Parallel change for Rental
As we did with the Movie class, we introduce a new Rental class with the parallel change strategy. After the rental data is read from user input, we create a new Rental object, taking movie and daysRented as its constructor parameters (line 14):
We use Alt + Enter on the constructor to have Resharper generate the class:
Note that we didn’t encapsulate the fields movie and daysRented yet: from the code, I am not sure yet if we will need access to those from outside the class. Since MainClass is using local variables for movie and daysRented, we know however that we need public access now. For this reason, I am making Rental‘s fields public, and will make them private if noone needs to access them after we move the behaviour into the Rental class. If access from outside is needed, I will encapsulate the field(s) with a public getter.
Preparing for narrow change
The goal now is to replace the usage of the individual variables movie and daysRented with accesses to our new class Rental.
Since those are used in several places inside the loop, the best is to do it with a narrow change: the idea behind it is to use an existing variable, to assign it something new (ie. the change we want to apply in several places), and then to inline that variable, so that our change takes effect everywhere where the variable was used.
The advantage is that we apply the change only once. It is also quicker and safer than using search and replace in the code: inlining the variable is applied only where it makes sense (scope of the variable). With search and replace, we need to manually check each occurrence and verify whether it’s safe or not to do the change.
In this example, we want to replace:
- movie with rental.movie
- daysRented with rental.daysRented
To do this, we need to have the following assignments:
Movie movie = rental.movie; int daysRented = rental.daysRented;
from the following code:
Watch out, now here is the trick:
The first thing we do is to inline the movie and daysRented given to the Rental constructor. To do so, use the refactoring Inline here while the caret is on movie / daysRented, in order to inline just this occurrence of the variable.
NOTE: unfortunately, the Inline here refactoring is not available in Resharper 8.2.3. We need to resort to copying the values by hand.
After this inlining, we move the instantiation of Rental from line 4 to line 2. After that, we can do our narrow change and assign the new values to both variables. We obtain the following:
Now we simply inline both these variables, and our changes are applied everywhere in the scope.
The loop looks like this:
Move behaviour to the new class
This parallel change to use Rental now allows us to move behaviour (methods) there. The responsibilities of calculating the frequent renter points and the bill amount belongs to Rental.
Extract method: GetAmount()
Let’s start with the calculation of the rental amount. We can extract a separate method from the big loop in MainClass with the Extract method refactoring:
A few points to note here:
When extracting the method, do not make it static: we want it to be an instance method of Rental.
Thanks to the previous preparation with the narrow change, this method now has a single parameter rental. This is exactly what we need to make the move to that class possible.
In this case, I already made the method public during the extraction because I know that MainClass will call it. It is also possible to leave it private while extracting and make it public with the move refactoring.
Move method: GetAmount()
We can now apply the refactoring Move instance method (Ctrl + R, O) to the method GetAmount().
When applying this refactoring, Resharper gives a list of possible targets. The possible targets are:
- the types present in the parameters of the method
- the types of the fields of the source class,
as long as those types belong to us and we can modify them (it wouldn’t give String as a possible destination, for example).
In this case, MainClass has no appropriate field (TextReader and TextWriter are part of the .NET framework). Hence, the only possible target type is our Rental class from the method’s parameter:
Extract method: GetFrequentRenterPoints()
The next thing we can extract from the while loop is the calculation of the frequent renter points.
int frequentRenterPoints = 0;
while (true)
{
// ...
// add frequent renter points
frequentRenterPoints++;
// add bonus for a two day new release rental
if (rental.movie.Category.Equals("NEW_RELEASE") && rental.daysRented > 1)
{
frequentRenterPoints++;
}
// ...
}
However, if we extract this piece of code, we get the following:
int frequentRenterPoints = 0;
while (true)
{
// ...
frequentRenterPoints = GetFrequentRenterPoints(frequentRenterPoints, rental);
// ...
}
private int GetFrequentRenterPoints(int frequentRenterPoints, Rental rental)
{
// add frequent renter points
frequentRenterPoints++;
// add bonus for a two day new release rental
if (rental.movie.Category.Equals("NEW_RELEASE") && rental.daysRented > 1)
{
frequentRenterPoints++;
}
return frequentRenterPoints;
}
This is not good. Because the same variable frequentRenterPoints is used for all rentals, we have to pass in the current state of the points in order to get the new value.
Actually, what this method should do is return the renter points for just this rental, and then we aggregate the points from all rentals to get the total in frequentRenterPoints.
The Trick
The trick is to introduce a new local variable in this scope. In this new local variable, we calculate the points for only the current rental in the loop. At the end of the code block, we add the value of this new variable to the total:
// add frequent renter points
int currentRenterPoints = 1; // new local variable
// add bonus for a two day new release rental
if (rental.movie.Category.Equals("NEW_RELEASE") && rental.daysRented > 1)
{
currentRenterPoints++;
}
frequentRenterPoints += currentRenterPoints; // move assignment to the end
Now, we can extract the method as follows:
while (true)
{
// add frequent renter points
frequentRenterPoints += GetFrequentRenterPoints(rental);
}
private int GetFrequentRenterPoints(Rental rental)
{
int currentRenterPoints = 1;
// add bonus for a two day new release rental
if (rental.movie.Category.Equals("NEW_RELEASE") && rental.daysRented > 1)
{
currentRenterPoints++;
}
return currentRenterPoints;
}
The method GetFrequentRenterPoints() is now ready to be moved to the Rental class.
Move method: GetFrequentRenterPoints()
After moving GetFrequentRenterPoints(), the while loop looks like this:
while (true)
{
// ...
decimal thisAmount = rental.GetAmount();
// add frequent renter points
frequentRenterPoints += rental.GetFrequentRenterPoints();
// show figures for this rental
result += "\t" + rental.movie.Name + "\t" + thisAmount.ToString("0.0", CultureInfo.InvariantCulture) + "\n";
totalAmount += thisAmount;
}
Cleaning up
Do not talk to strangers
When looking at the while loop above, we see a violation of the law of Demeter: MainClass is accessing rental.movie.Name! MainClass breaks encapsulation and knows details that are internal to Rental. To fix this, let’s extract a new method GetMovieName() and move it to Rental, where it belongs.
MainClass now accesses the rental object, instead of rental.movie:
result += "\t" + rental.GetMovieName() + "\t" + thisAmount.ToString("0.0", CultureInfo.InvariantCulture) + "\n";
Field visibility
When we go back to the Rental class, we see that we can now the two fields private. We don’t need to encapsulate them.
Rental now looks like this:
public class Rental
{
private readonly Movie movie;
private readonly int daysRented;
public Rental(Movie movie, int daysRented)
{
this.movie = movie;
this.daysRented = daysRented;
}
public decimal GetAmount()
{
decimal thisAmount = 0;
//determine amounts for rental
switch (movie.Category)
{
case "REGULAR":
thisAmount += 2;
if (daysRented > 2)
{
thisAmount += (daysRented - 2) * 1.5m;
}
break;
case "NEW_RELEASE":
thisAmount += daysRented * 3;
break;
case "CHILDRENS":
thisAmount += 1.5m;
if (daysRented > 3)
{
thisAmount += (daysRented - 3) * 1.5m;
}
break;
}
return thisAmount;
}
public int GetFrequentRenterPoints()
{
int currentRenterPoints = 1;
// add bonus for a two day new release rental
if (movie.Category.Equals("NEW_RELEASE") && daysRented > 1)
{
currentRenterPoints++;
}
return currentRenterPoints;
}
public string GetMovieName()
{
return movie.Name;
}
}
Summary
During this refactoring, we
- created a new class for rental-related responsibilities
- used the parallel change strategy to introduce the new class
- used narrow change to use the new Rental class everywhere the previous variables were used
- extracted methods and moved them to the new class
- introduced a local variable in order to be able to extract a method
The full code for this solved exercise is located here.
In the next step of this refactoring series, we will look at how to remove the responsibility of loading and creating Movies from the MainClass.
Pingback: Refactoring: introduce new class with Parallel Change | dom's data sharpening()
Pingback: Refactoring: split loop | dom's data sharpening()