This blog post will show you step by step to refactoring some code to be more readable (at least what I think). Patrik Löwnedahl gave me some of the ideas when we where talking about making code much cleaner. The following is an simple application that will have a list of movies (Normal and Transfer). The task of the application is to calculate the total sum of each movie and also display the price of each movie. class Program
{
enum MovieType
{
Normal,
Transfer
}
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfNormalMovie = 0;
int totalPriceOfTransferMovie = 0;
foreach (var movie in movies)
{
if (movie == MovieType.Normal)
{
totalPriceOfNormalMovie += 2;
Console.WriteLine("$2");
}
else if (movie == MovieType.Transfer)
{
totalPriceOfTransferMovie += 3;
Console.WriteLine("$3");
}
}
}
private static IEnumerable<MovieType> GetMovies()
{
return new List<MovieType>()
{
MovieType.Normal,
MovieType.Transfer,
MovieType.Normal
};
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
In the code above I’m using an enum, a good way to add types (isn’t it ;)). I also use one foreach loop to calculate the price, the loop has a condition statement to check what kind of movie is added to the list of movies. I want to reuse the foreach only to increase performance and let it do two things (isn’t that smart of me?! ;)).
First of all I can admit, I’m not a big fan of enum. Enum often results in ugly condition statements and can be hard to maintain (if a new type is added we need to check all the code in our app to see if we use the enum somewhere else). I don’t often care about pre-optimizations when it comes to write code (of course I have performance in mind). I rather prefer to use two foreach to let them do one things instead of two. So based on what I don’t like and Martin Fowler’s Refactoring catalog, I’m going to refactoring this code to what I will call a more elegant and cleaner code.
First of all I’m going to use Split Loop to make sure the foreach will do one thing not two, it will results in two foreach (Don’t care about performance here, if the results will results in bad performance, you can refactoring later, but computers are so fast to day, so iterating through a list is not often so time consuming.)
Note: The foreach actually do four things, will come to is later.
var movies = GetMovies();
int totalPriceOfNormalMovie = 0;
int totalPriceOfTransferMovie = 0;
foreach (var movie in movies)
{
if (movie == MovieType.Normal)
{
totalPriceOfNormalMovie += 2;
Console.WriteLine("$2");
}
}
foreach (var movie in movies)
{
if (movie == MovieType.Transfer)
{
totalPriceOfTransferMovie += 3;
Console.WriteLine("$3");
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
To remove the condition statement we can use the Where extension method added to the IEnumerable<T> and is located in the System.Linq namespace:
foreach (var movie in movies.Where( m => m == MovieType.Normal))
{
totalPriceOfNormalMovie += 2;
Console.WriteLine("$2");
}
foreach (var movie in movies.Where( m => m == MovieType.Transfer))
{
totalPriceOfTransferMovie += 3;
Console.WriteLine("$3");
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
The above code will still do two things, calculate the total price, and display the price of the movie. I will not take care of it at the moment, instead I will focus on the enum and try to remove them. One way to remove enum is by using the Replace Conditional with Polymorphism. So I will create two classes, one base class called Movie, and one called MovieTransfer. The Movie class will have a property called Price, the Movie will now hold the price:
public class Movie
{
public virtual int Price
{
get { return 2; }
}
}
public class MovieTransfer : Movie
{
public override int Price
{
get { return 3; }
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
The following code has no enum and will use the new Movie classes instead:
class Program
{
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfNormalMovie = 0;
int totalPriceOfTransferMovie = 0;
foreach (var movie in movies.Where( m => m is Movie))
{
totalPriceOfNormalMovie += movie.Price;
Console.WriteLine(movie.Price);
}
foreach (var movie in movies.Where( m => m is MovieTransfer))
{
totalPriceOfTransferMovie += movie.Price;
Console.WriteLine(movie.Price);
}
}
private static IEnumerable<Movie> GetMovies()
{
return new List<Movie>()
{
new Movie(),
new MovieTransfer(),
new Movie()
};
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
If you take a look at the foreach now, you can see it still actually do two things, calculate the price and display the price. We can do some more refactoring here by using the Sum extension method to calculate the total price of the movies:
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfNormalMovie = movies.Where(m => m is Movie)
.Sum(m => m.Price);
int totalPriceOfTransferMovie = movies.Where(m => m is MovieTransfer)
.Sum(m => m.Price);
foreach (var movie in movies.Where( m => m is Movie))
Console.WriteLine(movie.Price);
foreach (var movie in movies.Where( m => m is MovieTransfer))
Console.WriteLine(movie.Price);
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
Now when the Movie object will hold the price, there is no need to use two separate foreach to display the price of the movies in the list, so we can use only one instead:
foreach (var movie in movies)
Console.WriteLine(movie.Price);
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
If we want to increase the Maintainability index we can use the Extract Method to move the Sum of the prices into two separate methods. The name of the method will explain what we are doing:
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfMovie = TotalPriceOfMovie(movies);
int totalPriceOfTransferMovie = TotalPriceOfMovieTransfer(movies);
foreach (var movie in movies)
Console.WriteLine(movie.Price);
}
private static int TotalPriceOfMovieTransfer(IEnumerable<Movie> movies)
{
return movies.Where(m => m is MovieTransfer)
.Sum(m => m.Price);
}
private static int TotalPriceOfMovie(IEnumerable<Movie> movies)
{
return movies.Where(m => m is Movie)
.Sum(m => m.Price);
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
Now to the last thing, I love the ForEach method of the List<T>, but the IEnumerable<T> doesn’t have it, so I created my own ForEach extension, here is the code of the ForEach extension method:
public static class LoopExtensions
{
public static void ForEach<T>(this IEnumerable<T> values, Action<T> action)
{
Contract.Requires(values != null);
Contract.Requires(action != null);
foreach (var v in values)
action(v);
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
I will now replace the foreach by using this ForEach method:
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfMovie = TotalPriceOfMovie(movies);
int totalPriceOfTransferMovie = TotalPriceOfMovieTransfer(movies);
movies.ForEach(m => Console.WriteLine(m.Price));
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
The ForEach on the movies will now display the price of the movie, but maybe we want to display the name of the movie etc, so we can use Extract Method by moving the lamdba expression into a method instead, and let the method explains what we are displaying:
movies.ForEach(DisplayMovieInfo);
private static void DisplayMovieInfo(Movie movie)
{
Console.WriteLine(movie.Price);
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
Now the refactoring is done!
Here is the complete code:
class Program
{
static void Main(string[] args)
{
var movies = GetMovies();
int totalPriceOfMovie = TotalPriceOfMovie(movies);
int totalPriceOfTransferMovie = TotalPriceOfMovieTransfer(movies);
movies.ForEach(DisplayMovieInfo);
}
private static void DisplayMovieInfo(Movie movie)
{
Console.WriteLine(movie.Price);
}
private static int TotalPriceOfMovieTransfer(IEnumerable<Movie> movies)
{
return movies.Where(m => m is MovieTransfer)
.Sum(m => m.Price);
}
private static int TotalPriceOfMovie(IEnumerable<Movie> movies)
{
return movies.Where(m => m is Movie)
.Sum(m => m.Price);
}
private static IEnumerable<Movie> GetMovies()
{
return new List<Movie>()
{
new Movie(),
new MovieTransfer(),
new Movie()
};
}
}
public class Movie
{
public virtual int Price
{
get { return 2; }
}
}
public class MovieTransfer : Movie
{
public override int Price
{
get { return 3; }
}
}
pulbic static class LoopExtensions
{
public static void ForEach<T>(this IEnumerable<T> values, Action<T> action)
{
Contract.Requires(values != null);
Contract.Requires(action != null);
foreach (var v in values)
action(v);
}
}
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
I think the new code is much cleaner than the first one, and I love the ForEach extension on the IEnumerable<T>, I can use it for different kind of things, for example:
movies.Where(m => m is Movie)
.ForEach(DoSomething);
.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, "Courier New", courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }
By using the Where and ForEach extension method, some if statements can be removed and will make the code much cleaner. But the beauty is in the eye of the beholder. What would you have done different, what do you think will make the first example in the blog post look much cleaner than my results, comments are welcome!
If you want to know when I will publish a new blog post, you can follow me on twitter: http://www.twitter.com/fredrikn
