“Extreme” Refactoring — The Video Store Revisited

In Fowler’s Refactoring book, he has an extended example  of refactoring procedural code into object oriented code. I’ve gone through this example in talks a bunch of times, doing the refactorings Fowler does for the most part, with my own little twists to it.

Today, however, I wanted to have a little fun. I wanted to turn my refactoring knob up to about 12 and see what happened. So I started with the Customer class after it was refactored in the Video Store example and see how much further I could take things.

My goals were these:

  • Methods should be 1 line long. Anything longer than that needs to be looked at closely.
  • Aggressively eliminate all duplication, specifically duplication involved with iterating over containers several different times in similar ways
  • Use no private methods.

I had no idea if I could get to these points, but I thought it might be fun to try. So here’s how it went…

Starting Point

This is the Customer class as I started. This is the end point of this class in the Refactoring book, and it is the starting point from which I’m going to take off. (Not saying that Martin didn’t finish, but that I’m going further than a cost/benefit analysis might otherwise suggest :))

    public class Customer
    {
        private string name;
        private ArrayList rentals = new ArrayList();

        public Customer(string name)
        {
            this.name = name;
        }

        public void addRental(Rental rental)
        {
            rentals.Add(rental);
        }

        public string getName()
        {
            return name;
        }

        public string statement()
        {
            string result = “Rental Record for ” + getName() + System.Environment.NewLine;

            foreach (Rental each in rentals)
            {
                double thisAmount = each.CalculateRentalCost();

                result += “t” + each.getMovie().getTitle() + “t”
                    + thisAmount.ToString() + System.Environment.NewLine;
            }

            result += “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine;
            result += “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;

            return result;
        }

        private int CalculateTotalFrequentRenterPoints()
        {
            int frequentRenterPoints = 0;
            foreach (Rental each in rentals)
            {
                frequentRenterPoints += each.CalculateFrequentRenterPoints();
            }

            return frequentRenterPoints;
        }

        private double CalculateTotalRentalCost()
        {
            double totalAmount = 0.0;
            foreach (Rental each in rentals)
            {
                totalAmount += each.CalculateRentalCost();
            }
            return totalAmount;
        }
    }

Step 1 — Eliminate duplication in loops

So what struck me immediately about this code is that there is a 3x duplication in the looping over the Rental collection. I made one additional refactoring of the statement() method to show the third instance:

private string PrintLineItems()
{
    string result = “”;
    foreach (Rental each in rentals)
    {
        result += “t” + each.getMovie().getTitle() + “t”
            + each.CalculateRentalCost().ToString() + System.Environment.NewLine;
    }
    return result;
}

private int CalculateTotalFrequentRenterPoints()
{
    int frequentRenterPoints = 0;
    foreach (Rental each in rentals)
    {
        frequentRenterPoints += each.CalculateFrequentRenterPoints();
    }
    return frequentRenterPoints;
}

private double CalculateTotalRentalCost()
{
    double totalAmount = 0.0;
    foreach (Rental each in rentals)
    {
        totalAmount += each.CalculateRentalCost();
    }
    return totalAmount;
}

Now, I’d love to find a way to share this looping code. I know that this isn’t that big a deal, but, hey, this article is called “Extreme” Refactoring, right???? Part of the problem lies with the language here — looping in C++, Java, C#, and any other C-based language takes a lot of code. In Ruby, for instance, each of these loops would collapse down to a single line of code, and we’d be happy.

So, since we are using C#, we need to do something to share this code. And this is what we need to do. We need to replace the external iteration over the collection with a single iteration contained in some collection class that can be shared. I gave an example of this in a previous post, my TDD Deep Dive Part 4.

What I did is to create a strongly typed RentalCollection class and give it an Apply()-like method. This was a little tricky to do, and I’ll explain why in a second. The first step in the process was to do an ExtractClass to create the RentalCollection. I did a SelfEncapsulateField on rentals in Customer so that I could more easily change how Customer got to the rentals ArrayList. Then I created an instance of RentalCollection called rentalCollection, moved the ArrayList to the new class, and provided access to that collection through a public property, as such:

    public class Customer
    {
        private string name;
        private RentalCollection rentalCollection = new RentalCollection();

        private ArrayList Rentals
        {
            get { return rentalCollection.Rentals; }
            set { rentalCollection.Rentals = value; }
        }

        public Customer(string name)
        {
            this.name = name;
            Rentals = new ArrayList();
        }

        public void addRental(Rental rental)
        {
            Rentals.Add(rental);
        }

        private class RentalCollection
        {
            private ArrayList rentals;

            public ArrayList Rentals
            {
                get { return rentals; }
                set { rentals = value; }
            }
        }

Now that I have this, I have a place to put the shared loop that will help me get rid of the duplication in the Customer class. Step one in making this change is to create a single delegate that can be used to replace the body of each of those 3 foreach loops. This is actually harder than it seems since the types involved with all three loops are different, and we have to add and concatenate as we go. It took me a bit to get this right, but I tested it out by creating the delegate in the Customer class and replacing the loop bodies with calls to the delegate. This was the smallest step I could take:

        private delegate object Collector(object initialValue, Rental rental);

        private string PrintLineItems()
        {
            object result = “”;

            Collector collector = new Collector(LinePrinter);
            foreach (Rental each in Rentals)
            {
                object runningTotal = collector(result, each);
                result = runningTotal;
            }

            return (string)result;
        }

        private object LinePrinter(object initialValue, Rental rental)
        {
            string result = (string)initialValue;
            result += “t” + rental.getMovie().getTitle() + “t”
                + rental.CalculateRentalCost().ToString() + System.Environment.NewLine;

            return result;
        }

Then I factored out the part of the PrintLineItems() method that had nothing to do with the line items themselves into a method called Collect(), which, I’m hoping, will be reusable:

        private delegate object Collector(object initialValue, Rental rental);

        private string PrintLineItems()
        {
            Collector collector = new Collector(LinePrinter);
            string result = (string)Collect(collector, “”);

            return result;
        }

        private object Collect(Collector collector, object initialValue)
        {
            foreach (Rental rental in Rentals)
            {
                object runningTotal = collector(initialValue, rental);
                initialValue = runningTotal;
            }
            return initialValue;
        }

        private object LinePrinter(object initialValue, Rental rental)
        {
            string result = (string)initialValue;
            result += “t” + rental.getMovie().getTitle() + “t”
                + rental.CalculateRentalCost().ToString() + System.Environment.NewLine;

            return result;
        }

Now I can go ahead and change the other two methods to work like PrintLineItems() does:

private object Collect(Collector collector, object initialValue)
{
    foreach (Rental rental in Rentals)
    {
        object runningTotal = collector(initialValue, rental);
        initialValue = runningTotal;
    }
    return initialValue;
}

private string PrintLineItems()
{
    Collector collector = new Collector(LinePrinter);
    string result = (string)Collect(collector, “”);

    return result;
}

private object LinePrinter(object initialValue, Rental rental)
{
    string result = (string)initialValue;
    result += “t” + rental.getMovie().getTitle() + “t”
        + rental.CalculateRentalCost().ToString() + System.Environment.NewLine;

    return result;
}

private int CalculateTotalFrequentRenterPoints()
{
    Collector collector = new Collector(FrequentRenterPointsSummer);
    int frequentRenterPoints = (int)Collect(collector, 0);

    return frequentRenterPoints;
}

private object FrequentRenterPointsSummer(object initialValue, Rental rental)
{
    int frequentRenterPoints = (int)initialValue;
    frequentRenterPoints += rental.CalculateFrequentRenterPoints();

    return frequentRenterPoints;
}

private double CalculateTotalRentalCost()
{
    Collector collector = new Collector(RentalCostSummer);
    double rentalAmount = (double)Collect(collector, 0.0);

    return rentalAmount;
}

private object RentalCostSummer(object initialValue, Rental rental)
{
    double rentalAmount = (double)initialValue;
    rentalAmount += rental.CalculateRentalCost();

    return rentalAmount;
}

Step 2 — Move looping and delegate into RentalCollection

And we’ve now removed the loops from the individual methods and put it into the Collect() method, eliminating that duplication. The iteration logic and delegate really belong over in the RentalCollection class, so I’ll move them there:

public class RentalCollection
{
    private ArrayList rentals;

    public ArrayList Rentals
    {
        get { return rentals; }
        set { rentals = value; }
    }

    public delegate object Collector(object initialValue, Rental rental);

    public object Collect(Collector collector, object initialValue)
    {
        foreach (Rental rental in Rentals)
        {
            object runningTotal = collector(initialValue, rental);
            initialValue = runningTotal;
        }
        return initialValue;
    }
}

Step 3 — Getting rid of all those private methods

I have all the iteration out of the Customer class, but now I have to get rid of all those private methods. They are hidden nuggets of implementation that you can only see by looking at the code. I believe that if we make them public methods of another class, they will form another aspect of our design that will then be visible from the outside.

What I want to do is to move the two private methods for each item being collected into their own classes. One of those methods will become public, and one will have to remain private for reasons that will become apparent soon. I’m not initially going to give these three classes a base class because no one is going to initially use these polymorphically. Here is where this ends up:

private string PrintLineItems()
{
    LineItemPrinter printer = new LineItemPrinter(rentalCollection);
    return printer.PrintLineItems();
}

private int CalculateTotalFrequentRenterPoints()
{
    FrequentRenterPointsTotaller totaller = new FrequentRenterPointsTotaller(rentalCollection);
    return totaller.CalculateTotalFrequentRenterPoints();
}

private double CalculateTotalRentalCost()
{
    RentalCostTotaller totaller = new RentalCostTotaller(rentalCollection);
    return totaller.CalculateTotalRentalCost();
}

public class LineItemPrinter
{
    private readonly RentalCollection rentals;

    public LineItemPrinter(RentalCollection rentals)
    {
        this.rentals = rentals;
    }

    public string PrintLineItems()
    {
        RentalCollection.Collector collector = new RentalCollection.Collector(LinePrinter);
        string result = (string) rentals.Collect(collector, “”);

        return result;
    }

    private object LinePrinter(object initialValue, Rental rental)
    {
        string result = (string) initialValue;
        result += “t” + rental.getMovie().getTitle() + “t”
            + rental.CalculateRentalCost().ToString() + System.Environment.NewLine;

        return result;
    }
}

public class FrequentRenterPointsTotaller
{
    private readonly RentalCollection rentals;

    public FrequentRenterPointsTotaller(RentalCollection rentals)
    {
        this.rentals = rentals;
    }

    public int CalculateTotalFrequentRenterPoints()
    {
        RentalCollection.Collector collector = new RentalCollection.Collector(FrequentRenterPointsSummer);
        int frequentRenterPoints = (int) rentals.Collect(collector, 0);

        return frequentRenterPoints;
    }

    private object FrequentRenterPointsSummer(object initialValue, Rental rental)
    {
        int frequentRenterPoints = (int) initialValue;
        frequentRenterPoints += rental.CalculateFrequentRenterPoints();

        return frequentRenterPoints;
    }
}

public class RentalCostTotaller
{
    private readonly RentalCollection rentals;

    public RentalCostTotaller(RentalCollection rentals)
    {
        this.rentals = rentals;
    }

    public double CalculateTotalRentalCost()
    {
        RentalCollection.Collector collector = new RentalCollection.Collector(RentalCostSummer);
        double rentalAmount = (double) rentals.Collect(collector, 0.0);

        return rentalAmount;
    }

    private object RentalCostSummer(object initialValue, Rental rental)
    {
        double rentalAmount = (double) initialValue;
        rentalAmount += rental.CalculateRentalCost();

        return rentalAmount;
    }
}

What this has let us do is to get rid of the private methods inside Customer, except for the three mini-methods that invoke our new totalling classes. Those methods are OK, in my mind, since they are not places where logic exists — these methods exist merely to help explain the inner workings of our code. And there is one private method left in each of the new classes, but those methods have to be there to serve as targets for the delegates. I don’t think I can get rid of any of the private methods I just mentioned.

Step 4 — More optimization in Customer

Going back to the Customer class, let’s look at the private methods and see if I can make them into single-line methods, just for fun.

private string PrintLineItems()
{
    return new LineItemPrinter(rentalCollection).PrintLineItems();
}

private int CalculateTotalFrequentRenterPoints()
{
    return new FrequentRenterPointsTotaller(rentalCollection).CalculateTotalFrequentRenterPoints();
}

private double CalculateTotalRentalCost()
{
    return new RentalCostTotaller(rentalCollection).CalculateTotalRentalCost();
}

Not bad, and we’ll revisit the rest of the class later. Now lets look at anything we can share among the classes we just created

Step 5 — ExtractSuperclass on totalling classes

The first obvious thing we can do is to create a superclass and put the RentalCollection into the base class. Then all three of those classes can be made into subclasses of the new class, and their constructors can be changed to call the base constructor.

public class Collector
{
    private readonly RentalCollection rentals;

    protected RentalCollection Rentals
    {
        get { return rentals; }
    }

    public Collector(RentalCollection rentals)
    {
        this.rentals = rentals;
    }
}

and I’ll just show one of the derived classes:

public class RentalCostTotaller : Collector
{
    public RentalCostTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public double CalculateTotalRentalCost()
    {
        RentalCollection.Collector collector = new RentalCollection.Collector(RentalCostSummer);
        double rentalAmount = (double) Rentals.Collect(collector, 0.0);

        return rentalAmount;
    }

    private object RentalCostSummer(object initialValue, Rental rental)
    {
        double rentalAmount = (double) initialValue;
        rentalAmount += rental.CalculateRentalCost();

        return rentalAmount;
    }
}

Now we’ll make the delegate method of each of these classes have the same name. I’ll call them all DoCollect() instead of RentalCostSummer() for instance. This lets me move the creation of the Collector into the constructor of the base class. Note that I’m intentionally violating a .Net Design guideline here by using a method of a derived class in a constructor, but I know I’m doing it and it is a conscious tradeoff. Here is the Collector class:

public abstract class Collector
{
    private readonly RentalCollection rentals;
    private readonly RentalCollection.Collector collectDelegate;

    protected RentalCollection.Collector CollectDelegate
    {
        get { return collectDelegate; }
    }

    protected RentalCollection Rentals
    {
        get { return rentals; }
    }

    public Collector(RentalCollection rentals)
    {
        this.rentals = rentals;
        this.collectDelegate = new RentalCollection.Collector(DoCollect);
    }

    protected abstract object DoCollect(object initialValue, Rental rental);
}


and one of the derived classes:

public class RentalCostTotaller : Collector
{
    public RentalCostTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public double CalculateTotalRentalCost()
    {
        double rentalAmount = (double) Rentals.Collect(CollectDelegate, 0.0);
        return rentalAmount;
    }

    protected override object DoCollect(object initialValue, Rental rental)
    {
        double rentalAmount = (double) initialValue;
        rentalAmount += rental.CalculateRentalCost();

        return rentalAmount;
    }
}

Now we can rewrite the CalculateTotalRentalCost() method to be a little more generic, which will allow us to refactor out a bit more code:

public class RentalCostTotaller : Collector
{
    public RentalCostTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public double CalculateTotalRentalCost()
    {
        return (double)PerformCollection(0.0);
    }

    protected override object DoCollect(object initialValue, Rental rental)
    {
        double rentalAmount = (double) initialValue;
        return rentalAmount + rental.CalculateRentalCost();
    }
}

with Collector turning into:

public abstract class Collector
{
    private readonly RentalCollection rentals;
    private readonly RentalCollection.Collector collectDelegate;

    protected RentalCollection.Collector CollectDelegate
    {
        get { return collectDelegate; }
    }

    protected RentalCollection Rentals
    {
        get { return rentals; }
    }

    public Collector(RentalCollection rentals)
    {
        this.rentals = rentals;
        this.collectDelegate = new RentalCollection.Collector(DoCollect);
    }

    protected object PerformCollection(object initialValue)
    {
        return Rentals.Collect(CollectDelegate, initialValue);
    }

    protected abstract object DoCollect(object initialValue, Rental rental);
}

Step 6 — Simplifying everything

Now I think we’re about there as far as classes go, so lets run back around our classes and see if we can’t simplify the code and create some one-line methods.

Collector and each derived class can be simplified:

public abstract class Collector
{
    private readonly RentalCollection rentals;
    private readonly RentalCollection.Collector collectDelegate;

    public Collector(RentalCollection rentals)
    {
        this.rentals = rentals;
        this.collectDelegate = new RentalCollection.Collector(DoCollect);
    }

    protected object PerformCollection(object initialValue)
    {
        return rentals.Collect(collectDelegate, initialValue);
    }

    protected abstract object DoCollect(object initialValue, Rental rental);
}

Nothing bug one-liners there. And now each of the derived classes can be simplified to:

public class RentalCostTotaller : Collector
{
    public RentalCostTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public double CalculateTotalRentalCost()
    {
        return (double)PerformCollection(0.0);
    }

    protected override object DoCollect(object rentalCost, Rental rental)
    {
        return (double)rentalCost + rental.CalculateRentalCost();
    }
}

Step 7 — Final simplifications in Customer class

There are some simplifications that I still see in the Customer class. I think there is still structural duplication in the three private methods that I can make a little bit better by creating static methods on each of the Collector classes. Those private methods are now:

private string PrintLineItems()
{
    return LineItemPrinter.Collect(rentalCollection);
}

private int CalculateTotalFrequentRenterPoints()
{
    return FrequentRenterPointsTotaller.Collect(rentalCollection);
}

private double CalculateTotalRentalCost()
{
    return RentalCostTotaller.Collect(rentalCollection);
}

I think that’s a bit better. Not sure why it is better, but I think it is. I think. Now let’s look back at the statement() method of this class:

public string statement()
{
    string result = “Rental Record for ” + getName() + System.Environment.NewLine;

    result += PrintLineItems();

    result += “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine;
    result += “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;

    return result;
}

We can simplify this to:

public string statement()
{
    string result = PrintHeader();
    result += PrintLineItems();
    result += PrintFooter();

    return result;
}

private string PrintFooter()
{
    string result = “”;
    result += “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine;
    result += “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;
    return result;
}

private string PrintHeader()
{
    return “Rental Record for ” + getName() + System.Environment.NewLine;
}

private string PrintLineItems()
{
    return LineItemPrinter.Collect(rentalCollection);
}

and statement() can be further simplified to :

public string statement()
{
    return PrintHeader() + PrintLineItems() + PrintFooter();
}

and PrintFooter() to :

private string PrintFooter()
{
    return “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine +
           “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;
}

At this point, Customer has only single-line methods in it, and just those three explaining private methods:

public class Customer
{
    private string name;
    private RentalCollection rentalCollection = new RentalCollection();

    private ArrayList Rentals
    {
        get { return rentalCollection.Rentals; }
        set { rentalCollection.Rentals = value; }
    }

    public Customer(string name)
    {
        this.name = name;
        Rentals = new ArrayList();
    }

    public void addRental(Rental rental)
    {
        Rentals.Add(rental);
    }

    public string getName()
    {
        return name;
    }

    public string statement()
    {
        return PrintHeader() + PrintLineItems() + PrintFooter();
    }

    private string PrintFooter()
    {
        return “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine +
               “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;
    }

    private string PrintHeader()
    {
        return “Rental Record for ” + getName() + System.Environment.NewLine;
    }

    private string PrintLineItems()
    {
        return LineItemPrinter.Collect(rentalCollection);
    }

    private int CalculateTotalFrequentRenterPoints()
    {
        return FrequentRenterPointsTotaller.Collect(rentalCollection);
    }

    private double CalculateTotalRentalCost()
    {
        return RentalCostTotaller.Collect(rentalCollection);
    }
}

In looking at this class, I can get rid of the Rentals property by adding an Add method to RentalCollection, simplifying Customer a bit more. But the important thing to notice here is that I have several methods that kind of seem to belong together, in a place by themselves.

The contents of the statement() method and the three PrintXXX() methods seem to belong to a RentalStatement class. This is a class whose job it is to produce the rental statement. Let’s do that refactoring and see where it takes us :

public class RentalStatement
{
    private readonly RentalCollection rentals;
    private readonly string customerName;

    public RentalStatement(RentalCollection rentals, string customerName)
    {
        this.rentals = rentals;
        this.customerName = customerName;
    }

    public string GenerateStatement()
    {
        return PrintHeader() + PrintLineItems() + PrintFooter();
    }

    private string PrintFooter()
    {
        return “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine +
            “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;
    }

    private string PrintHeader()
    {
        return “Rental Record for ” + customerName + System.Environment.NewLine;
    }

    private string PrintLineItems()
    {
        return LineItemPrinter.Collect(rentals);
    }

    private int CalculateTotalFrequentRenterPoints()
    {
        return FrequentRenterPointsTotaller.Collect(rentals);
    }

    private double CalculateTotalRentalCost()
    {
        return RentalCostTotaller.Collect(rentals);
    }
}

In factoring out the RentalStatement class, I had to take along the CalculateTotalFrequentRenterPoints() and CalculateTotalRentalCost() methods as well. I’m not entirely sure that I want these methods in this class, but if I keep them in the Customer class then I have some processing of the RentalCollection in that class and some in this class. I also have to pass in the result of these calculations into either the constructor or the GenerateStatement() method, and I’m not sure I like that either. This is a judgement call.

The Customer class is reduced to this:

public class Customer
{
    private string name;
    private RentalCollection rentalCollection = new RentalCollection();

    public Customer(string name)
    {
        this.name = name;
    }

    public void addRental(Rental rental)
    {
        rentalCollection.Add(rental);
    }

    public string getName()
    {
        return name;
    }

    public string statement()
    {
        return new RentalStatement(rentalCollection, getName()).GenerateStatement();
    }
}

Conclusion

With this, I believe I’m finished. I’ve taken the original Customer class and applied some interesting refactorings to it. I’ve gotten rid of all private methods that do anything more than just explain a single line of code, and those are just in the RentalStatement class. Every method is a single line except for some constructors for classes with more than one member variable, except for the loop in the RentalCollection class. And there is no duplication anywhere that I can see.

Now for the questions. Is this code more clear than the original Customer class? I’m not sure. In some senses it is. If I want to go see how the statement is generated, I can now go look at a RentalStatement class and see it. Looking ahead a bit, knowing that one of the changes Martin talks about in his book is that we need to support an HTML statement as well as a text statement, so by creating the RentalStatement class, we’re well prepared to make that change. I’m never sure if the ReplaceExternalIterationWithInternalIteration ever makes code more clear, but it does eliminate duplication.

I’m not sure that I would ever go this far in real life, but it is nice to know that I could.

What do you think?

Here is the final code:

public class Customer
{
    private string name;
    private RentalCollection rentalCollection = new RentalCollection();

    public Customer(string name)
    {
        this.name = name;
    }

    public void addRental(Rental rental)
    {
        rentalCollection.Add(rental);
    }

    public string getName()
    {
        return name;
    }

    public string statement()
    {
        return new RentalStatement(rentalCollection, getName()).GenerateStatement();
    }
}

public class RentalCollection
{
    private ArrayList rentals = new ArrayList();

    public ArrayList Rentals
    {
        get { return rentals; }
        set { rentals = value; }
    }

    public delegate object Collector(object initialValue, Rental rental);

    public object Collect(Collector collector, object initialValue)
    {
        foreach (Rental rental in Rentals)
        {
            object runningTotal = collector(initialValue, rental);
            initialValue = runningTotal;
        }
        return initialValue;
    }

    public void Add(Rental rental)
    {
        rentals.Add(rental);
    }
}

public abstract class Collector
{
    private readonly RentalCollection rentals;
    private readonly RentalCollection.Collector collectDelegate;

    public Collector(RentalCollection rentals)
    {
        this.rentals = rentals;
        this.collectDelegate = new RentalCollection.Collector(DoCollect);
    }

    protected object PerformCollection(object initialValue)
    {
        return rentals.Collect(collectDelegate, initialValue);
    }

    protected abstract object DoCollect(object initialValue, Rental rental);
}

public class LineItemPrinter : Collector
{
    public static string Collect(RentalCollection rentals)
    {
        return new LineItemPrinter(rentals).PrintLineItems();
    }

    public LineItemPrinter(RentalCollection rentals) : base(rentals)
    {
    }

    public string PrintLineItems()
    {
        return (string)PerformCollection(“”);
    }

    protected override object DoCollect(object initialValue, Rental rental)
    {
        return (string)initialValue + “t” + rental.getMovie().getTitle() + “t”
            + rental.CalculateRentalCost().ToString() + System.Environment.NewLine;
    }
}

public class FrequentRenterPointsTotaller : Collector
{
    public static int Collect(RentalCollection rentals)
    {
        return new FrequentRenterPointsTotaller(rentals).CalculateTotalFrequentRenterPoints();
    }

    public FrequentRenterPointsTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public int CalculateTotalFrequentRenterPoints()
    {
        return (int)PerformCollection(0);
    }

    protected override object DoCollect(object frequentRenterPoints, Rental rental)
    {
        return (int)frequentRenterPoints + rental.CalculateFrequentRenterPoints();
    }
}

public class RentalCostTotaller : Collector
{
    public static double Collect(RentalCollection rentals)
    {
        return new RentalCostTotaller(rentals).CalculateTotalRentalCost();
    }

    public RentalCostTotaller(RentalCollection rentals) : base(rentals)
    {
    }

    public double CalculateTotalRentalCost()
    {
        return (double)PerformCollection(0.0);
    }

    protected override object DoCollect(object rentalCost, Rental rental)
    {
        return (double)rentalCost + rental.CalculateRentalCost();
    }
}

public class RentalStatement
{
    private readonly RentalCollection rentals;
    private readonly string customerName;

    public RentalStatement(RentalCollection rentals, string customerName)
    {
        this.rentals = rentals;
        this.customerName = customerName;
    }

    public string GenerateStatement()
    {
        return PrintHeader() + PrintLineItems() + PrintFooter();
    }

    private string PrintFooter()
    {
        return “You owed ” + CalculateTotalRentalCost().ToString() + System.Environment.NewLine +
            “You earned ” + CalculateTotalFrequentRenterPoints().ToString() + ” frequent renter points” + System.Environment.NewLine;
    }

    private string PrintHeader()
    {
        return “Rental Record for ” + customerName + System.Environment.NewLine;
    }

    private string PrintLineItems()
    {
        return LineItemPrinter.Collect(rentals);
    }

    private int CalculateTotalFrequentRenterPoints()
    {
        return FrequentRenterPointsTotaller.Collect(rentals);
    }

    private double CalculateTotalRentalCost()
    {
        return RentalCostTotaller.Collect(rentals);
    }
}

— bab

21 thoughts to ““Extreme” Refactoring — The Video Store Revisited”

  1. I’d pass in the complete Customer object to the report generator. That way, if another future reports need more information from a customer than just their name, then they can.

  2. Wow fun stuff. Great post.

    I agree with your "smell" of moving the CalculateTotalRentalCost and CalculateTotalFrequentRenterPoints methods into the RentalStatement class…

    I’d move them to the RentalCollection class where they really seem to belong.

    I totally get the point of the post, and what a great morning read this was. Keep it up.

  3. Darron, I hadn’t thought of putting them in the RentalCollection class, but that is interesting. Let me think about that. We’re having our monthly xpstl meeting tonight, and I’m presenting this there. I’ll blog about any changes that we come up with, and I’ll add your feedback to that of the group tonight.

    Steve, also an interesting idea. I’m a little reluctant to pass the Customer around, as that creates another dependency, but it may be the best answer. I’ll think about that and mention it at the meeting tonight as another option.

    Thanks to you all for the feedback. I really do appreciate it.

  4. Brian and about twenty-one of his closest (pun intended) extreme programming friends gathered tonight in St. Peters MO to discuss this example. We covered this entire exercise in just shy of two hours with lively discussion. We had the same “starting point” but ended up with a slightly different solution (this is from memory): RentalCollection became abstract and ArrayList was removed, and delegates were removed and replaced with polymorphic behavior. In my opinion, the exercise was worthwhile and I would recommend that you share this with your colleagues; you just might have a slightly different solution too.

  5. Removing the delgates actually turned out to be pretty easy, but it reduces the generality of the solution. The delegates were originally in there because the three loops that I was looking to share were all in the same class, the Customer class. Given that, I had to be able to specify the name of the method to pass as the loop body.

    Once the refactoring happened that created the new hierarchy of classes, each encapsulating one of the loop bodies, I could replace the delegate by a call to the abstract method defined in Collector. Does that make sense?

    </p>

    The downside of this is that it is less correct idiomatically for .Net. There seems to be a preference for delegates over inheritance hierarchies in .Net, which would lead me to leave the delegate in there and accept the extra complexity for the extra flexibility.

    </p>

    If this still doesn’t make sense, please let me know, and I’ll do a separate blog entry showing the refactoring from delegate to method invocation

    </P>

    –bab

  6. I’m not sure I understand the 1-line-long goal. Although I agree that methods should be short and to the point, it seems that in the example the GenerateStatement method can only accomplish this goal because the return types from the three methods it calls return a type that can be concatenated.

    For example, if the PrintXXX() methods actually caused printing to a device and returned nothing (void), the GenerateStatement would become (and as a result would likely be renamed PrintStatement):

    PrintHeader();

    PrintLineItems();

    PrintFooter();

    Is this bad?

    There are several areas in the Framework and other non-modifiable code that require such procedural actions to occur within a single method in order to accomplish the method’s stated purpose. How could something like that be refactored with these goals in mind?

    I would like to see an extension of this example in which profiling showed that all of the string concatenations were significantly reducing performance and needed to be replaced with StringBuilders where appropriate…with the same three stated goals (1 line, no duplication, and no or few privates).

  7. Hi, Shawn,

    Thanks for reading and taking the time to comment. The point of the exercise, to me, was not to develop a style that I would always use on my code. It was to see where I could get to if I took everything to its extreme, logical conclusion.

    In taking this refactoring further than anyone else ever had before, I created an entirely new class hierarchy, which enabled me move procedural logic into better places, where better is defined as small, well named, public methods on classes.

    I agree with your assessment of the Statement() method being just fine as a ComposedMethod with three lines in it — if that is the best I can do, then that is really good code. It just so happened that in this context, I was able to take things farther.

    Since writing this article, I have certainly found many places that cannot be refactored down to a single line of code, but this experience has made me much less tolerant of procedural code and *any* form of code duplication, and this wouldn’t have happened to me unless I had taken this example to its extreme end, showing me just what is possible.

    BTW, I could have accomplished the same thing with a string builder by using it as a collecting parameter to each of the statement methods, and returning it from each of them as well, as return GenerateFooter(GenerateBody(GenerateHeader(new StringBuilder()))).ToString();

    Didn’t say I liked it, but I could certainly do it!

    — bab

  8. Thanks again for the original post. This has certainly made me start examining my own code more closely for inefficiencies and duplication.

    If I understand this correctly, the 1-line long goal, as with the other goals, are laudable as goals that I should strive for in my code, but not always acheivable.

    I can expect this to be especially true of binary components with which I need to interact that have, for example, setup-action-teardown or other defined order of execution semantics.

    Thanks again for a great post that kick-started my brain.

  9. I really don’t get the point here. Why take something simple and comprehensible and turn it into something complicated and incomprehensible, while also reducing performance?

    I’d say, just move the loop bodies from the private methods into the first loop. That will give you less codelines, better performance and simpler code.

    foreach (Rental each in rentals)

    {

    double thisAmount = each.CalculateRentalCost();

    frequentRenterPoints += each.CalculateFrequentRenterPoints();

    totalAmount += each.CalculateRentalCost();

    }

  10. Mike,

    Thanks for the comments. Remember what my overriding goal was for this exercise — to take things in a different direction and farther than I had before. I was just stretching my refactoring muscles to see what I could learn.

    My tactical goals were all methods limited to a single line, no private methods that were not mandated by the language, and all collections encapsulated with functor-like interfaces to them exposed.

    That was the direction I was headed, and your solution, while certainly elegant and performant, just wasn’t heading in the same direction as I was looking.

    As I said, I don’t know that I would go as far as this in real code, but I learned some things by doing this exercise. And that was my intent.

    — bab

  11. I absolutely loved this exercise. The only thing is, I didn’t like this code:

    public object Collect(Collector collector, object initialValue)

    {

    foreach (Rental rental in Rentals)

    {

    object runningTotal = collector(initialValue, rental);

    initialValue = runningTotal;

    }

    return initialValue;

    }

    I tend to favor something like:

    public object Collect(Collector collector, object initialValue)

    {

    object result = initialValue;

    foreach (Rental rental in Rentals)

    result = collector(result, rental);

    return result;

    }

  12. This is a good example of the "Goldilocks" educational pattern that I read on the net somewhere (this pattern was written by Kent Beck, by the way). Goldilocks tried the chair that was too big, too small, and just right, the porridge that was too hot, too code, and just right, and the bed that was too hard, too soft, and just right.

    To find out what is "just right", explore the extremes… one-line methods, no private methods, etc. The other extreme — very large methods, lots of private methods, etc., you can also explore, though you’ve probably experienced it already in legacy code.

    Good write-up!

  13. public Customer(string name)

    {

    this.name = name;

    Rentals = new ArrayList();

    }

    public void addRental(Rental rental)

    {

    Rentals.Add(rental);

    }

    public string getName()

    {

    return name;

    got it here..haha

Leave a Reply to Shawn Cancel reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.