Category Archives: Refactoring

Refactoring: extract creation logic into factory

This step of the command line video store refactoring series (1, 2 and 3) is pretty straightforward: we will further simplify the loop in MainClass by moving creation logic into a separate class.

The code for starting this exercise is here. The end of this step is there as well.

The state of affairs

The Run() method of the MainClass still contains a loop having several responsibilities:

while (true)
{
    string input = _in.ReadLine();
    if (string.IsNullOrEmpty(input))
    {
        break;
    }
    string[] rentalData = input.Split(' ');
    var rental = new Rental(movieRepository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));

    // add frequent renter points
    frequentRenterPoints += rental.GetFrequentRenterPoints();

    // show figures for this rental
    result += "\t" + rental.GetMovieName() + "\t" + rental.GetAmount().ToString("0.0", CultureInfo.InvariantCulture) + "\n";
    totalAmount += rental.GetAmount();
}

The line creating the Rental object stands out by its relative complexity:

var rental = new Rental(movieRepository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));

All by itself, it does the following:

  1. parses the entered rental data
  2. fetches the corresponding Movie
  3. creates the Rental

Due to the several responsibilities involved, we will have to modify MainClass if the low-level parsing of the rental data changes. Let’s prevent this by moving this logic to a separate class that takes care of this.

Introduce a RentalFactory class

Extract method

The first step is to extract a method (Ctrl + R, M) out the loop for the Rental creation statement. Since I know this method will get moved elsewhere, I’m making it public already:

public Rental CreateRental(string[] rentalData)
{
    var rental = new Rental(movieRepository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
    return rental;
}

However, this new method used the field movieRepository, which is only accessible in MainClass. To circumvent this, we can pass the repository by applying Introduce Parameter (Ctrl + R, P) on the field:

public Rental CreateRental(string[] rentalData, MovieRepository repository)
{
    var rental = new Rental(repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
    return rental;
}

Create class RentalFactory

We have two alternatives for creating this new class:

  1. add a new field of type RentalFactory to MainClass and let Resharper generate an empty class for us (see example)
  2. use Extract Class on CreateRental()

The first option, that we used in the previous step, generates a new, empty class, to which we can then move our method. The second has the advantage of creating the new class and moving the method in one go.

Let’s do it.

Point to the method CreateRental() and choose Extract Class (Ctrl + Shift + R, E).

extract-class

MainClass now looks cleaner:

public MainClass(TextReader @in, TextWriter @out)
{
    _out = @out;
    _in = @in;
    rentalFactory = new RentalFactory();
}

// ...

Rental rental = RentalFactory.CreateRental(rentalData, movieRepository);

The generated RentalFactory class:

public class RentalFactory
{
    public RentalFactory()
    {
    }

    public Rental CreateRental(string[] rentalData, MovieRepository repository)
    {
        var rental = new Rental(repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
        return rental;
    }
}

We got this done with minimal manual typing, nice.

However, we still have a little work to do:

public class MainClass
{
    // ...
    private readonly MovieRepository movieRepository = new MovieRepository();
    private readonly RentalFactory rentalFactory;
    
    // why the public getter???
    public RentalFactory RentalFactory
    {
        get
        {
            return rentalFactory;
        }
    }
    // ...
}

When extracting the RentalFactory class, Resharper created a new field, and encapsulated it with a public getter. This is not necessary, and I actually do not want the factory to be available outside the class.

Let’s get rid of the getter by applying Inline Method (Ctrl + R, I) to it:

Rental rental = rentalFactory.CreateRental(rentalData, movieRepository);

Does anyone know how to prevent Resharper from creating this getter? I did not find any setting which could deactivate this behaviour.

Change parameter to field

When looking at the rentalFactory.CreateRental() call above, we see that we need to pass in a MovieRepository so that the Rental can be created.

It would be better to pass the repository into the factory into the constructor, and not having to worry about it in subsequent CreateRental() calls.

To do so, let’s go into RentalFactory, and apply Introduce Field (Ctrl + R, F) on the repository parameter:

public class RentalFactory
{
    private MovieRepository repository;

    public RentalFactory()
    {
    }

    public Rental CreateRental(string[] rentalData, MovieRepository repository)
    {
        this.repository = repository;
        var rental = new Rental(repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
        return rental;
    }
}

The field is assigned in the CreateRental() method, which is not ideal, but we can leave it like that for the moment.

Also, note the empty constructor, which is convenient to have. We are now going to add a new constructor where we can inject the MovieRepository. To do so, we will use the code snippet ctorf:

code-snippet-ctorf

This snippet creates a new constructor that takes all the classes’ fields as parameters and initialises them in one go. Quite handy to have.

public class RentalFactory
{
    private MovieRepository repository;

    public RentalFactory(MovieRepository repository)
    {
        this.repository = repository;
    }

    public RentalFactory()
    {
    }

    public Rental CreateRental(string[] rentalData, MovieRepository repository)
    {
        this.repository = repository;
        var rental = new Rental(repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
        return rental;
    }
}

If we didn’t have the explicit empty constructor, we would have a compile error at this point, because MainClass calls the RentalFactory constructor without parameter.

Now, we can go to the usage of that empty constructor and insert the movieRepository.

public MainClass(TextReader @in, TextWriter @out)
{
    _out = @out;
    _in = @in;
    rentalFactory = new RentalFactory(movieRepository);
}

Cleanup

Like after every refactoring, we have some cleanup to do before calling it a day.

MainClass

The initialisation of the MovieRepository and RentalFactory are not consistent: the former is initialised at its declaration, whereas the latter is in the constructor.

Let’s make it consistent by moving the initialisation of the MovieRepository field to the constructor. Resharper offers a way to do it automagically by pressing Alt + Enter while the caret is either before or after the ‘=’ sign:

move-initialisation-to-constructor

public class MainClass
{
    // ...
    private readonly MovieRepository movieRepository;
    private readonly RentalFactory rentalFactory;
    // ...
    public MainClass(TextReader @in, TextWriter @out)
    {
        _out = @out;
        _in = @in;
        movieRepository = new MovieRepository();
        rentalFactory = new RentalFactory(movieRepository);
    }
    // ...
}

RentalFactory

There are a few things we can improve in the factory:

public class RentalFactory
{
    private MovieRepository repository;

    public RentalFactory(MovieRepository repository)
    {
        this.repository = repository;
    }

    public RentalFactory()
    {
    }

    public Rental CreateRental(string[] rentalData, MovieRepository repository)
    {
        this.repository = repository;
        var rental = new Rental(repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
        return rental;
    }
}

Remove unused empty constructor

The parameterless constructor is empty and is never called. Remove it with Alt + Enter or Alt + Del.

Remove the first line of CreateRental()

CreateRental() still gets the MovieRepository passed as a parameter, and uses this, instead of the field, to fetch the movie. This is useless because the field gets initialised in the constructor.

We can safely delete the redundant field assignment. Additionally, we use field this.repository instead of parameter repository when creating the Rental:

public Rental CreateRental(string[] rentalData, MovieRepository repository)
{
    var rental = new Rental(this.repository.GetByKey(int.Parse(rentalData[0])), int.Parse(rentalData[1]));
    return rental;
}

Remove unused parameter repository from CreateRental()

The parameter repository is now unused and can be removed: Alt + Enter or Alt + Del on it.
After this, we can also remove the redundant this qualifier on the field (Alt + Enter).

Rename field repository to movieRepository

Let’s make the name of the field describe itself better by renaming it from repository to movieRepository. We do this by using the Rename refactoring on it (Ctrl + R, R) on the field. In the dialog box, choose the option to also rename related symbols, so that the constructor parameter gets renamed as well.

Rename method CreateRental() to CreateFrom()

The name of the method unnecessarily repeats the word ‘Rental’: we do not need to say it again when we are working with a class named RentalFactory. Let’s rename the method to CreateFrom() by using Ctrl + R, R on it.

That way, the naming makes more sense on the caller side: the factory creates the rental from the data.

rentalFactory.CreateFrom(rentalData);

Introduce local variables for movie and daysRented

The Rental constructor takes two parameters, and at the moment, it is not clear which parameter is what. We can the code more self-describing by introducing two local variables with sensible names.

Let’s introduce a new local variable on the first constructor parameter (Ctrl + R, V), and name it movie. Note that we can take advantage of the autocomplete the Resharper provides us to choose the name.

Now we do the same for the second parameter (Ctrl + R, V) and use the suggested name daysRented.

Movie movie = movieRepository.GetByKey(int.Parse(rentalData[0]));
int daysRented = int.Parse(rentalData[1]);

Note that we could take it further and introduce another variable for the key handed to the movieRepository. But since it is already pretty obvious that it is a key by just looking at the name of the method GetByKey(), we can leave it as is with a clean coder conscience.

Inline local variable rental

This is probably a matter of taste, but when we create a variable and do nothing else with it than to just return it, I prefer to simply inline that variable and return the corresponding expression.

This assumes however that the expression is simple enough to be understood on its own, without the need to clarify it by using a descriptive variable name.

This is the case here, it is obvious that we are instantiating a new Rental object.

We hence apply Inline Variable (Ctrl + R, I).

Make field movieRepository readonly

Again a matter of taste: field movieRepository can be made readonly.

I like to mark fields as readonly, because it gives the guarantee that the field is not going to be modified anywhere.

This also prevents the intentional or deliberate introduction of temporary field smells.

Are we clean now?

After performing the cleanup steps above, we are left with the following code for the RentalFactory:

public class RentalFactory
{
    private readonly MovieRepository movieRepository;

    public RentalFactory(MovieRepository movieRepository)
    {
        this.movieRepository = movieRepository;
    }

    public Rental CreateFrom(string[] rentalData)
    {
        Movie movie = movieRepository.GetByKey(int.Parse(rentalData[0]));
        int daysRented = int.Parse(rentalData[1]);
        return new Rental(movie, daysRented);
    }
}

In my opionion, the code is minimal, simple, and self-explanatory. Any suggestions for further improvements?

Summary

In this exercise, we removed the responsibility of parsing and creating rentals from the main class to a new factory class.

We improved the code by:

  • extracting methods
  • extracting a class
  • introducing a field
  • using code snippets
  • introducing local variables to make the code better speak for itself

The code for the end of this exercise can be found here.

In the next refactoring step, we will again use split loop to separate the remaining responsibilities from the main while loop.

Refactoring: split loop

This is the third episode of the command line video store refactoring exercises (1, 2).

The focus of this step is to see how to split loops (and other control structures like if) in order to be able to extract and separate the concerns.

The state of the code at the beginning of the exercise is here, and the code from the end of the exercise there.

The Problem

Currently, MainClass is responsible for the loading, the creation and the retrieval of the Movies. This is not ideal because it has too many responsibilities. MainClass should act as a controller and define the high-level processes of the video store and should not be burdened with additional tasks such as loading and providing access to the store’s movies.

MainClass loads and creates the Movies:

// read movies from file
var movies = new Dictionary<int, Movie>();
using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
using (BufferedStream bs = new BufferedStream(fs))
using (StreamReader reader = new StreamReader(bs))
{
    while (!reader.EndOfStream)
    {
        string line = reader.ReadLine();
        string[] movieData = line.Split(';');
        var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
        movies.Add(movie.Key, movie);

        _out.WriteLine(movie.Key + ": " + movie.Name);
    }
}

Further in MainClass, the movies dictionary is used to find the movie to pass on the Rental‘s constructor.

while (true)
{
    // ...
    string[] rentalData = input.Split(' ');
    var rental = new Rental(movies[int.Parse(rentalData[0])], int.Parse(rentalData[1]));
    // ...
}

The solution to The Problem

The task of providing access to domain objects is typically done by a repository. We will introduce a new class MovieRepository that will take care of loading and retrieving the Movies.

Several concerns in the loop

When we take a look at the while loop that reads user input and creates the Movies, we see that there is a second concern being handled: output to the user.

while (!reader.EndOfStream)
{
    string line = reader.ReadLine();
    string[] movieData = line.Split(';');
    var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
    movies.Add(movie.Key, movie);

    _out.WriteLine(movie.Key + ": " + movie.Name);
}

Because of this, we cannot extract the loading from the display. When wanting to separate different concerns from a control structure such as a loop, we use the split loop pattern.

Split loop

The idea behind split loop is to duplicate the control structure at hand (once for every concern). Then, in each loop, we remove the duplicate concerns until there is only one concern per loop.

Typically, the first loop constructs a collection over which the duplicated loops will then iterate.

In this example, we have two concerns in a while loop. This loop builds a dictionary of Movies. To separate the display concern from the creation, we will add a new loop over the Movies that will take care of the display.

// read movies from file
var movies = new Dictionary<int, Movie>();
using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
using (BufferedStream bs = new BufferedStream(fs))
using (StreamReader reader = new StreamReader(bs))
{
    while (!reader.EndOfStream)
    {
        string line = reader.ReadLine();
        string[] movieData = line.Split(';');
        var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
        movies.Add(movie.Key, movie);
    }
    
}
// duplicated loop on the same collection
foreach (Movie movie in movies.Values)
{
    _out.WriteLine(movie.Key + ": " + movie.Name);
}

The code above shows how we split the loop in two: we added a new foreach loop over movies and moved the _out.WriteLine(); statement over there.

Now that each loop manages one concern, we can extract methods correspondingly.

Extract methods

After splitting the loop, we can extract the behaviours into separate methods. At the moment I am only interested in the loading of the Movies, so I will not extract a method for displaying them yet. I’ll call the loading method GetAll().

Another thing I am extracting a method for is the lookup in the movies dictionary when we create Rentals, which I’ll call GetByKey().

public Dictionary<int, Movie> GetAll()
{
    var movies = new Dictionary<int, Movie>();
    using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
    using (BufferedStream bs = new BufferedStream(fs))
    using (StreamReader reader = new StreamReader(bs))
    {
        while (!reader.EndOfStream)
        {
            string line = reader.ReadLine();
            string[] movieData = line.Split(';');
            var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
            movies.Add(movie.Key, movie);
        }
    }
    return movies;
}

public Movie GetByKey(Dictionary<int, Movie> movies, int key)
{
    return movies[key];
}

Note that I made the methods public and non-static, which will make it straightforward to move them to another class later.

Move the methods to a new class

Now that we have prepared methods for the new repository, we need to create a new class. We need to keep in mind that we want to move the methods GetByKey() and GetAll() to this new class.

As we saw previously, Resharper suggests possible target classes when moving instance methods: fields and parameters of the methods.

In this case, we have either no parameter (GetAll()) or types that do not belong to our code (GetByKey(Dictionary<TKey, TValue>, int)). The remaining possibility is to have a field from a class that we have control on.

Create new class as a field

In order to prepare for the move, we need to create a new class. As mentioned before, we need a field in our class to provide a move target for the methods we extracted earlier.

MovieRepository is added as a field. Let Resharper generate the class from this statement (Alt + Enter):

public class MainClass
{
    private readonly TextReader _in;
    private readonly TextWriter _out;
    private readonly MovieRepository movieRepository = new MovieRepository(); // move target
    // ...
}

Move instance methods

Use the refactoring Move instance method (Ctrl + R, O) on methods GetAll() and GetByKey() and move them to MovieRepository.

public class MovieRepository
{
    public Dictionary<int, Movie> GetAll()
    {
        var movies = new Dictionary<int, Movie>();
        using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
        using (BufferedStream bs = new BufferedStream(fs))
        using (StreamReader reader = new StreamReader(bs))
        {
            while (!reader.EndOfStream)
            {
                string line = reader.ReadLine();
                string[] movieData = line.Split(';');
                var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
                movies.Add(movie.Key, movie);
            }
        }
        return movies;
    }

    public Movie GetByKey(Dictionary<int, Movie> movies, int key)
    {
        return movies[key];
    }
}

Cleaning up

The MovieRepository class is now operational, but there is still some cleaning up to do.

Load the Movies once

The GetAll() method loads the movies from the file each time it is called. Let’s improve this by moving the loading code to the constructor and storing the Movies in a field that will be reused by the repository.

With the caret on the movies variable in GetAll(), and press Ctrl + R, F to Introduce field. Choose non-static and to intialise the field in the constructor.

Introduce field

We now have a movies field that is instantiated in a new constructor.

The next thing to do is to cut the using block in GetAll() and paste it into the constructor.

We obtain the following code:

public class MovieRepository
{
    private readonly Dictionary<int, Movie> movies;

    public MovieRepository()
    {
        movies = new Dictionary<int, Movie>();
        using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
        using (BufferedStream bs = new BufferedStream(fs))
        using (StreamReader reader = new StreamReader(bs))
        {
            while (!reader.EndOfStream)
            {
                string line = reader.ReadLine();
                string[] movieData = line.Split(';');
                var movie = new Movie(int.Parse(movieData[0]), movieData[1], movieData[2]);
                movies.Add(movie.Key, movie);
            }
        }
    }
    
    public Dictionary<int, Movie> GetAll()
    {
        return movies;
    }
    
    // ...
}

Parameter hides field

In the GetBykey() method, you can see that the parameter movies hides the field with the same name. Moreover, callers of this method should not have to provide the movies dictionary themselves.

public Movie GetByKey(Dictionary<int, Movie> movies, int key)
{
    return movies[key];
}

Disambiguate by qualifying the field with this.

public Movie GetByKey(Dictionary<int, Movie> movies, int key)
{
    return this.movies[key];
}

We can now remove the unused parameter by pressing Alt + Enter on it, Remove parameter (and update usages). Also remove the now unnecessary this qualifier.

public Movie GetByKey(int key)
{
    return movies[key];
}

Encapsulate Dictionary<>

The fact that MovieRepository uses a Dictionary to internally store the Movie is an implementation that we want to hide so that it can be changed when needed, with no impact on the users of the class.

Hence, the GetAll() method should not return a dictionary, but a list of Movies. That way, the consumers of the repository do not have to access the dictionary’s Values property, which will make usage more readable and simpler.

Let’s fix this by having GetAll() return movies.Values.ToList() instead of movies. Use Alt + Enter on the now erroneous return statement and choose Change type of method to List<Movie>:

public List<Movie> GetAll()
{
    return movies.Values.ToList();
}

After that, the usage in MainClass can be simplified to the following:

foreach (Movie movie in movieRepository.GetAll())
{
    _out.WriteLine(movie.Key + ": " + movie.Name);
}

Summary

In this exercise, we saw how to:

  • split loops (and other control structures) by duplicating them
  • introduce a field in the source class for the target type where we want to move instance methods to

The technique to split loops can be used also for other control flow structures like if, switch and such.

The full code for the end of this exercise is located there.

In the next exercise, we will continue to move responsibilities out of MainClass.

Refactoring: introduce new class and move behaviour to it

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):

decimal totalAmount = 0;
int frequentRenterPoints = 0;
string result = "Rental Record for " + customerName + "\n";
while (true)
{
    string input = _in.ReadLine();
    if (string.IsNullOrEmpty(input))
    {
        break;
    }
    string[] rentalData = input.Split(' ');
    Movie movie = movies[int.Parse(rentalData[0])];
    int daysRented = int.Parse(rentalData[1]);
    var rental = new Rental(movie, daysRented); // parallel change

    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;
    }

    // add frequent renter points
    frequentRenterPoints++;
    // add bonus for a two day new release rental
    if (movie.Category.Equals("NEW_RELEASE") && daysRented > 1)
    {
        frequentRenterPoints++;
    }
    // show figures for this rental
    result += "\t" + movie.Name + "\t" + thisAmount.ToString("0.0", CultureInfo.InvariantCulture) + "\n";
    totalAmount += thisAmount;
}

We use Alt + Enter on the constructor to have Resharper generate the class:

public class Rental
{
    public readonly Movie movie;
    public readonly int daysRented;

    public Rental(Movie movie, int daysRented)
    {
        this.movie = movie;
        this.daysRented = daysRented;
    }
}

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:

string[] rentalData = input.Split(' ');
Movie movie = movies[int.Parse(rentalData[0])];
int daysRented = int.Parse(rentalData[1]);
var rental = new Rental(movie, daysRented); // 'inline here' on both parameters

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:

string[] rentalData = input.Split(' ');
var rental = new Rental(movies[int.Parse(rentalData[0])], int.Parse(rentalData[1]));
// narrow change on both variables
Movie movie = rental.movie;
int daysRented = rental.daysRented;
// now we can inline the two variables above to apply the changes everywhere

Now we simply inline both these variables, and our changes are applied everywhere in the scope.

The loop looks like this:

while (true)
{
    string input = this._in.ReadLine();
    if (string.IsNullOrEmpty(input))
    {
        break;
    }
    string[] rentalData = input.Split(' ');
    var rental = new Rental(movies[int.Parse(rentalData[0])], int.Parse(rentalData[1]));

    decimal thisAmount = 0;
    //determine amounts for rental
    switch (rental.movie.Category)
    {
        case "REGULAR":
            thisAmount += 2;
            if (rental.daysRented > 2)
                thisAmount += (rental.daysRented - 2)*1.5m;
            break;
        case "NEW_RELEASE":
            thisAmount += rental.daysRented*3;
            break;
        case "CHILDRENS":
            thisAmount += 1.5m;
            if (rental.daysRented > 3)
                thisAmount += (rental.daysRented - 3)*1.5m;
            break;
    }

    // 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++;
    }
    // show figures for this rental
    result += "\t" + rental.movie.Name + "\t" + thisAmount.ToString("0.0", CultureInfo.InvariantCulture) + "\n";
    totalAmount += thisAmount;
}

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:
Move instance method

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.

Refactoring: introduce new class with Parallel Change

This post is the first in a series of refactorings of the command line video store exercise. You can find the original version in Jave on Rick’s repository. I will be working on the corresponding C# version.

The goal of this series is to start with procedural code and turn it into object-oriented code. This series presumes you are using Resharper.

This exercise is divided into 8 steps. In this post, I will go over the first step. You can find the full code on the E1 branch. When this step is done, the code looks like the one on the E2 branch.

In this step, we will introduce a new Movie class and move the corresponding data there.

Before you start, I suggest you take a look at the full main class here.

The movies are loaded from a file:

public void Run()
{
    // read movies from file
    var movies = new Dictionary<int, string[]>();
    using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
    using (BufferedStream bs = new BufferedStream(fs))
    using (StreamReader reader = new StreamReader(bs))
    {
        while (!reader.EndOfStream)
        {
            string line = reader.ReadLine();
            string[] movie = line.Split(';');
            movies.Add(int.Parse(movie[0]), movie);
            _out.WriteLine(movie[0] + ": " + movie[1]);
        }
    }
    // ...
}

The code uses a dictionary to store the movies by an integer key. You can see that a movie is represented by a primitive structure: an array of string. This array contains the following information about a movie:

  • movie[0]: the key
  • movie[1]: the name
  • movie[2]: the category

We want to replace this structure (primitive obession smell) with a Movie class that contains the data above.

To do so, we will proceed by introducing a parallel change: create a new dictionary of Movie instead of string[].

The key to the parallel change is to find all writes to the old structure and duplicating those writes to the new structure. When all writes also go to the new structure, we can start replacing the reads from the old structure to the new one.

Introduce Movie class

To introduce the Movie class, duplicate the movies variable (Ctrl + D) to movies1 Dictionary<int, Movie>. Since the Movie class does not exist yet, the code does not compile. We will fix that soon.

In the loop where the movies dictionary is filled, we add a new Movie to movies1. The Movie constructor takes the three elements from the string array as parameters: key, name and category.

Once this is done, we are ready to generate the Movie class. Place the cursor on the Movie constructor and press Alt + Enter, Create class ‘Movie’ to let Resharper create a class for us:

Create Movie

Note that we introduced a local variable for key to avoid having the parse code three times inside the loop, and that we renamed the movie[] array to movieData[] for more clarity.

Now that we have the Movie class, we have to remove the NotImplementedException from the constructor.

By default, Resharper will create the new class inside the current file, so we will need to use Alt + Enter on the class name to move the class to a separate file.

We create and initialise fields by hitting Alt + Enter on each constructor parameter and by choosing the option Introduce and intialise field.

Since we know that these fields will be used by the MainClass, we can encapsulate them so that we have public getters. This is the result:

Now the code compiles again and the tests are still green.

Parallel change: replace reads

The next step is to replace the reads on the old string array structure with our new Movie accessors. In the while loop, we replace all reads to movie[] with the corresponding property from the movie1 instance of Movie.

Every thing looks good, the tests are still green. Now we can remove the reads and writes to the old structure, since we are now using the new one everywhere.

We notice that movie[] is not used anymore in the while loop: remove it. The old movies dictionary is now only used to add the old string array to it. Remove it as well. Now that the old structure is gone, we can rename movies1 and movie1 to movies and movie:

public void Run()
{
    // read movies from file
    var movies = new Dictionary<int, Movie>();
    using (FileStream fs = File.Open(@"movies.cvs", FileMode.Open, FileAccess.Read))
    using (BufferedStream bs = new BufferedStream(fs))
    using (StreamReader reader = new StreamReader(bs))
    {
        while (!reader.EndOfStream)
        {
            string line = reader.ReadLine();
            string[] movieData = line.Split(';');
            int key = int.Parse(movieData[0]);
            var movie = new Movie(key, movieData[1], movieData[2]);
            movies.Add(key, movie);

            _out.WriteLine(movie.Key + ": " + movie.Name);
        }
    }
    // ...
    while (true)
    {
        string input = _in.ReadLine();
        if (string.IsNullOrEmpty(input))
        {
            break;
        }
        string[] rental = input.Split(' ');
        Movie movie = movies[int.Parse(rental[0])];
        decimal thisAmount = 0;

        int daysRented = int.Parse(rental[1]);
        //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;
        }

        // add frequent renter points
        frequentRenterPoints++;
        // add bonus for a two day new release rental
        if (movie.Category.Equals("NEW_RELEASE") && daysRented > 1)
        {
            frequentRenterPoints++;
        }
        // show figures for this rental
        result += "\t" + movie.Name + "\t" + thisAmount.ToString("0.0", CultureInfo.InvariantCulture) + "\n";
        totalAmount += thisAmount;
    }
    // ...
}

There is no behaviour that we can move to the Movie class, so we will leave it at that for this step.

Summary

In this step, we saw how to:

  • recognise the code smell primitive obsession.
  • quickly creating a new class by typing “new MyClass(parameters)” and using Resharper to generate the class.
  • using the parallel change strategy:
    1. introduce a new structure,
    2. duplicate all writes to the old structure for the new structure,
    3. replace reads from the old structure to use the new structure,
    4. remove the old structure.

You can find the full code on this branch.

After this refactoring, the MainClass is still pretty messy and does too many different things. In the next step, we will introduce a new class for the concept of rental.