Refactoring Q & A: When Should I Refactor?

Answer: When you have to spend too long figuring out what the code does.

Consider the following partial implementation of a ShoppingCart class.

public class ShoppingCart
{
    public ShoppingCart()
    {
        this._readOnlyDetails = new ReadOnlyCollection<OrderDetail>(_details);
    }

    private readonly List<OrderDetail> _details = new List<OrderDetail>();
    private readonly ReadOnlyCollection<OrderDetail> _readOnlyDetails;
    public ReadOnlyCollection<OrderDetail> Details
    {
        get
        {
            return _readOnlyDetails;
        }
    }

Our ShoppingCart class is simply hiding the internal storage mechanism of Details so that clients of ShoppingCart cannot add or remove products directly. For this reason, ShoppingCart must provide its own methods for modifying the OrderDetails collection. In this post I implemented this method and applied some heavy refactoring. Don’t read the implementation just yet.

Consider the requirements: When adding a product to the shopping cart, I must provide an OrderDetail that indicates the quantity of that product that has been ordered. Subsequent additions of the same product to the shopping cart should not result in additional order details. Rather, the existing OrderDetail should reflect the sum of all quantities of the product being ordered.

Imagine that you open the source file to see how this is done, but you have all the outlining collapsed so that all you see is method signatures. Imagine that these are the method signatures you see:

public void AddProduct(Product product, int quantity)
private OrderDetail FindOrCreateDetailForProduct(Product product)
private OrderDetail CreateDetailForProduct(Product product)
private OrderDetail FindDetailByProductSku(string productSku)
private bool HasDetailForProductSku(string productSku)

Just read each one of these method signatures. Each method states clearly exactly what it is going to do. Now for each of the method signatures, imagine that you were going to expand the outlining so you can see the method body. What code would you expect to find in each one. Seriously, pause for a moment and think about what you would expect to find in each method body. Got it? Okay, now read the actual implementations of these methods.

public void AddProduct(Product product, int quantity)
{
    var detail = FindOrCreateDetailForProduct(product);
    detail.Quantity += quantity;
}

private OrderDetail FindOrCreateDetailForProduct(Product product)
{
    var detail = HasDetailForProductSku(product.Sku) ?
        FindDetailByProductSku(product.Sku) :
        CreateDetailForProduct(product);
    return detail;
}

private OrderDetail CreateDetailForProduct(Product product)
{
    var detail = new OrderDetail() { Product = product };
    this._details.Add(detail);
    return detail;
}

private OrderDetail FindDetailByProductSku(string productSku)
{
    var detail = this.Details.Single(d => d.Product.Sku == productSku);
    return detail;
}

private bool HasDetailForProductSku(string productSku)
{
    return this.Details.Any(d => d.Product.Sku == productSku);
}

I’m sure that some of the implementations are different than you imagined, but are they different in any substantial way? Probably not. A method called HasDetailForProductSku can have only so many syntactic implementations, all of which are semantically identical. The method names and signatures state what our code is doing in English-language terms that are meaningful to a person. The bodies of these methods provide the syntax that is meaningful to the compiler. Class, method, parameter, and variable names are targeted at people. Their purpose is to express the intent of the code to people. Conditionals, loops, and other statements are the basic forms in which we code, but their target audience is the compiler. To put this in the form of a general principle: “Refactor when the semantic meaning of the code is hidden or obscured by the syntax required by the compiler,” or “Refactor when you have to spend too long figuring out what the code does.”

Leave a Reply