Brian Button - One Agile Coder

This blog is moving -- http://www.agileprogrammer.com/oneagilecoder

Peter Provost and I have decided to start our own blogging site, http://www.agileprogrammer.com. This site is going to be dedicated to discussing agile programming in a .Net context. And I am moving my blog there.

If you are accessing this site through http://oneagilecoder.agilestl.com, that will still work, and will get you to the new place.

If you are accessing the site through http://www.dotnetjunkies.com/oneagilecoder, you’ll need to change to http://www.agileprogrammer.com/oneagilecoder. That will be my new blogging home.

Hope to see you all there!

— bab

TDD -- What about internal methods?

Over the past few months, I’ve taken part in a few chats about whether or not internal methods should be tested. One of the basic premises of TDD is that you should use it to explore, design, and implement the public interface of your classes. If you are using it to test methods that are less than public, you are probably testing implementation and not interface, which leads to very fragile tests.

For the longest time I didn’t believe that. I truly believed that internal methods were just another “public”-like interface, to a smaller audience. And I just spent 30 minutes writing this blog entry explaining my reasoning. And as I went through my arguments, I started thinking. And then I erased what I had written.

So, this is my new post:

A few weeks ago I posted an entry called Extreme Refactoring. One of the goals of what I did was to eliminate all private methods, just to see where that took the design. I liked where it went, and it has changed how I think about new code that I write.

Maybe, must maybe, we can do the same kind of thing with internal methods. Private methods are an indication that a class is doing too much. It is possible to move that logic out of the private methods into public methods of other classes. And maybe it is possible that a class that has both public and internal methods is doing too much as well. Maybe that’s a hint that that class is doing too much.

At this point I don’t know. I really meant this posting to be an explanation of why writing internal methods through TDD made sense, since they weren’t really anything other than scoped publics. But now I’m starting to wonder. I just don’t know. I’m going to have to try to write code cognizant of this and see where it takes me. Maybe in well defined and factored classes, you have either private, protected, and public methods. I haven’t decided if internal classes are OK or not — I’ll have to try it and find out.

The problem I have seen is that in large applications, there are services that classes offer to each other, inside the bounds of the application, that classes outside the application should not be able to consume. Internal methods go part of the way towards solving this, but even they break down when the application spans multiple assemblies. I want to find a way to address this issue, but still create safe, security-conscious code.

Sorry for the ramble, but this was really an exploration and journey for me to get to this point. I look forward to trying this theory out.

bab

Now playing: Liquid Tension Experiment - Liquid Tension Experiment 2 - Liquid Dreams

Enterprise Library released to the wild!!!

I know I’m a few days late to this party, but I’ve been recovering from workaholism for the past few days. Now, feeling refreshed and energetic, I add my voice to the growing firestorm:

Check out the Enterprise Library. It is everything you have ever hoped for, dreamed about, and wanted, for your software development needs :)

Compliments cheerfully accepted. Criticisms consciously sought and appreciated. After all, we want to get better.

bab

Now playing: Dream Theater - Falling Into Infinity - Burning My Soul

Death by spam

I admit it. Spam has me beat.

A few weeks ago, I got around 1000 spam a day.

Two weeks ago, I got 2000 spams a day.

Last week, 3000 per day.

Are you picking up the pattern yet? As of today, I got 4500 spams. By next week, I'll be over 5000, I just know it.

Does anyone out there have any suggestions about how to tame this problem? I have spambayes in the middle of my mailtool chain, so I don't see very many of these, but I still have to deal with the bandwidth they take up. And I still see over 100 per day.

My requirements are that I want to be able to read my mail on any of my machines, and I want to have the email backed up safely nightly. I also have to control the mailboxes that I use. I currently have 100 or so active email addresses on my domain, so I need to be able to create and delete addresses at will.

Any advice? Please? I'm begging here.

UPDATE — My predictions were far too conservative. I got 7500 spams yesterday. I gotta get some help. This is email spam, and I want to get rid of it so I never see it. I really want something to sit between fetchmail on my linux box and my POP account to sift through and get rid of my spam before I download it. I really don’t want challenge/response solutions, because that places an undo burden on people sending me business email. I’d also be happy with a different mail service that still looked like agilestl.com, but did all the filtering for me server side. Suggestions are most welcome…

-- bab

A constructor is not part of a class's interface

Peter Provost and I were talking today about some code that we’re working on together. We’re constructing a class right now, called a Starter. The job of a Starter is to stitch together a Repository, a class whose job it is interact with a source control repository, subversion in our case, a Builder, which is responsible for causing a Project to be built, and a BuildLogger. All of this stuff has to be aware of configuration changes, so we also have to pass in a Configuration object that is able to tell us the current configuration values. The constructor signature looks like this:

public Starter(IStarterConfiguration, IRepository, IBuilder, IBuildLogger) // excuse the shorthand

When I write code, I typically have constructors that look like this. I believe that it is not the job of the class to know where its bits and pieces come from, because this would tightly couple the Starter class to specific types of configs, repositories, etc, which is a bad thing. So, I frequently have a different class, like a factory or a builder whose job it is to construct the objects and send them along their merry way. Creating code like this lets me keep the methods of the class as clean as possible, and allows me to vary the actual type of objects passed in.

The downside of this is that construction of my objects can get a little complicated. My constructors can end up taking several different kinds of objects that are really only exposed because I want to avoid the tight coupling. But I don’t consider this to be a problem.

Others do not share my opinion.

I believe that the interface of the class are only that class’s member functions and properties. Constructors are not part of the interface. Constructors never appear in an interface, and are not what typical clients use. They just call the regular methods.

It turns out that this style of creating classes creates systems that are amenable to dependency injection techniques, both manually and through using tools like pico. This lets you create flexible systems at the price of exposing some of your implementation through constructor arguments. I haven’t played with tools like this yet, but I’m starting to get the itch…

Sorry for the rambling, but that’s what was on my mind today.

— bab

Release date for Enterprise Library announced!

According to Scott Densmore, barring anything unpredictable happening, Version 1.0 of the Enterprise Library will be released to the web on January, 28th. It will be available MSDN and the patterns & practices site.

Yahoo!!!!

<ShamelessPlug>
Hmmm. Rumor has it that Agile Solutions Group will soon be offering training classes on Enterprise Library as well as consulting services revolving around using it. Email entlib@agilesolutionsgroup.com if you’re interested :)
</ShamelessPlug>

— bab

Now playing: Rush - Power Windows - Marathon

"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

"Cannot Create SSPI Context" and how to fix it

Just a quick note for anyone who has ever gotten this error before: “Cannot create SSPI context”. I started getting this error when I was trying to establish a connection to a SQL Server when I wasn’t connected to the internal Microsoft corporate network. Connection strings that worked just fine when I was connected just wouldn’t work at all, and I could not figure out how to fix it.

GIYF (Google Is Your Friend) I googled it and found out that I had my connection string set wrong. I was using localhost for the server name in the connection string, which is not correct. I needed to use the SQL Server name, not the hostname.

So I changed the server to brian-thinkpad, my machine name, and everything worked just fine.

Hopefully this will save someone else the hour or so of confusion I spent trying to fix this.

— bab

Enterprise Library talk at St. Louis C# Users Group, January 10th

I’ll be speaking about the Enterprise Library project I’ve been working on at Microsoft since early 2004 at the C# UG in St. Louis on Monday, January 10th.

I’ll be talking about the design of the Enterprise Library, showing off some of its capabilities, and demonstrating its common configuration tooling. Given time, I’ll also be building a sample application, to illustrate how simple it is to consume it in your own application.

For time, location, and more information, see http://www.stlcsharp.net.