Entities and the Law of Demeter

The Law of Demeter, and its corresponding code smell, Inappropriate Intimacy, are some of the best bang-for-your-buck code smells that you can address.  The basic idea behind each of these concepts is code related to an object should probably be inside that object.  It's also known as the Principle of Least Knowledge, and I've found it very helpful in creating objects with rich behavior.

It's not always something I catch immediately, but I tend to notice later.  For example, I noticed this code in one of my code behinds:

protected decimal GetTotal()
{
    return _cart
             .GetItems()
             .Sum(item => item.Price * item.Quantity);
}

This method was then called by markup in the template file to display the current total in the cart.

Now, it seems rather obvious that something like the cart's total should belong on the ShoppingCart object itself.  In some situations, there might not even be a ShoppingCart class, only a generic list of ShoppingCartItems.  In that case, the best thing to do would be to create the ShoppingCart class, if nothing else than to become a magnet for cart behavior.

With the Move Method refactoring, I moved this behavior to the ShoppingCart class:

public class ShoppingCart
{
    private readonly List<ShoppingCartItem> _items;

    public ShoppingCart()
    {
        _items = new List<ShoppingCartItem>();
    }

    public ShoppingCartItem[] GetItems()
    {
        return _items.ToArray();
    }

    public void AddItem(Product product, int quantity)
    {
        var item = new ShoppingCartItem(product);
        item.Quantity = quantity;
        _items.Add(item);
    }

    public decimal GetTotal()
    {
        return _items.Sum(item => item.Price * item.Quantity);
    }

}

Again, it seems like more total behavior is in the wrong place.  Why should ShoppingCart have to perform the price/quantity calculation?  Let's move this to the ShoppingCartItem class:

public class ShoppingCartItem
{
    private readonly Product _product;

    public ShoppingCartItem(Product product)
    {
        _product = product;
    }

    public int Quantity { get; set; }

    public decimal Price
    {
        get { return _product.Price; }
    }

    public Product Product
    {
        get { return _product; }
    }

    public decimal GetItemTotal()
    {
        return Quantity * Price;
    }
}

I also noticed several other places that cared way too much about the Product class.  Here's an example I found in the code-behind:

protected decimal CalculateMargin(ShoppingCartItem item)
{
    return item.Product.Price / item.Product.Cost;
}

Again, this is probably obvious to a lot of people (except me).  This many dots poking into the ShoppingCartItem means the code behind class cares much too deeply about the inner workings of the ShoppingCartItem.  Something like a margin seems like a fundamental concept of a Product, so let's just move it there:

public class Product
{
    public string Name { get; set; }
    public decimal Price { get; set; }
    public decimal Cost { get; set; }

    public decimal CalculateMargin()
    {
        return Cost == 0 ? 0m : Price / Cost;
    }
}

I would also create a delegating method on ShoppingCartItem, so callers wouldn't need to poke around the insides of Product to find what they needed.  I find it best to expose exactly what's needed on the container class, creating an explicit contract with the callers and telling them exactly what they should care about.

The behavior is out there

In many legacy code applications, I find lots of examples of anemic domain models, where the "business objects" have zero behavior and lots of properties.  Essentially, they just become containers of data.  One step above DataSets, but that's it.

Lots of duplication exists around these business objects, where I find the same methods and snippets poking around the business objects, getting the information they need.  These helper methods often have nothing to do with the class they're in, as we saw with the CalculateMargin method.  They do something related to some other class, but for whatever reason, don't exist on that class.

These data container classes don't have any behavior on them, but their behavior is out there somewhere, scattered among the rest of the application.  Moving behavior concerning a type actually to that type creates very cohesive domain models.  Centralizing data and behavior into a single type creates powerful domain models and a much richer client experience.

---

Posted Jul 07 2008, 07:51 AM by bogardj