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.
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.
Pingback: Refactoring: introduce new class and move behaviour to it | dom's data sharpening()