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:

parses the entered rental data
fetches the corresponding Movie
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:

add a new field of type RentalFactory to MainClass and let Resharper generate an empty class for us (see example)
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).

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:

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:

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.

dom's data sharpening

data persistence in .NET, clean code, refactoring

Refactoring: extract creation logic into factory