David Hayden [MVP C#]

.NET Tutorials, Patterns, and Practices

Information Expert GRASP Pattern Take 2

I am beating the Information Expert GRASP Pattern to death, but Thomas Eyde had a great comment about how one of the code smells I left in the Create a Shopping Cart example actually violated the Information Expert principle. He's right, and it is one of the code smells I mentioned in the original shopping cart post.

The code in question is as follows:

ShoppingCart Class

public class ArrayListCart : ShoppingCart
{
    private ArrayList _items;
    
    // ...
    
    public override void AddItem(Item item)
    {
        int index = _items.IndexOf(item);

        if (index >= 0)
            UpdateItem(item);
        else
            _items.Add(new Item(item.ProductID, item.ProductName,
                item.Quantity, item.UnitPrice));
    }
    
    // ...
    
}

In the AddItem method above, the ArrayListCart class "makes a copy" of item before placing it in the _items ArrayList. As I mentioned in the Creat a Shopping Cart post, this is not ideal (bogus actually) as ArrayListCart is not the Information Expert on what it takes to make a copy of Item. Item is. Therefore, as Thomas so well put it, Item should have a Copy() method to make a copy of itself.

ShoppingCart Class

public class ArrayListCart : ShoppingCart
{
    private ArrayList _items;
    
    // ...
    
    public override void AddItem(Item item)
    {
        int index = _items.IndexOf(item);

        if (index >= 0)
            UpdateItem(item);
        else
            _items.Add(item.Copy());
    }
    
    // ...
    
}

Item's Copy Method

public Item Copy()
{
   return new Item(this._productID, this._productName,
        this._quantity, this._unitPrice);
}

Now we have a much cleaner solution that follows the Information Expert GRASP Pattern. Although you don't have to follow these principles as if they are written in stone, you can see that they may help show problems in your code. Check out Applying UML and Patterns for more information on GRASP Patterns.

Published Mar 27 2005, 09:56 AM by David Hayden

Filed under: GRASP Patterns

Comments

Karel Vandenhove said:

Very interesting articles.
I read the GRASP book a while ago (first edition). Is it worthwile buying ed3?
btw, Doesn't the UpdateItem method belong to Item as well?

public override void AddItem(Item item)
{
int index = _items.IndexOf(item);

if (index >= 0)
_items[index].Update(item);
else
_items.Add(new Item(item.ProductID, item.ProductName,
item.Quantity, item.UnitPrice));
}

public override void UpdateItem(Item item)
{
int index = _items.IndexOf(item);

if (index >= 0)
{
Item currentItem = _items[index] as Item;
currentItem.Update(item);
}
else
throw new ArgumentException(
String.Format("Item = {0} is not in the shoppingcart.", item));
}

# March 27, 2005 10:26 PM

Mark Bonafe said:

Please pardon my ignorance, it is vast. Why not just add the item (i.e. _items.Add(item))? Why create a new item?

# March 28, 2005 5:35 AM

David Hayden said:

Karel,

I only have the 3rd edition, so I am not sure if it would be money well spent to buy the new edition or not. The preface says that the book has been updated since the 2nd edition to include UML2, agile modeling, test-driven development, refactoring, iterative development, and a few more examples and case studies.

As for whether item should update itself, I think you are absolutely right. I totally miss that and had once again violated the Information Expert principle. I definitely need to come out with another version of the shopping cart.

Excellent comment!

# March 28, 2005 6:42 AM

David Hayden said:

Hi Mark,

You don't have to make a copy. You could just add the item into the arraylist.

As a rule of thumb, certainly not written in stone, it is ideal not to have objects outside your classes to have internal references to those classes.

If you give developers access to internal references, they could start modifying internal classes and collections without using your properties and methods (API), causing all kinds of interesting "problems."

For example, we want the shopping cart to only have 1 entry in the _items ArrayList per ProductID. When someone adds another item, we check to see if the ProductID's are the same and either update an existing item's quantity or add a new item in the _items ArrayList.

Let's say we don't make a copy of the item and just add it to _items. Now, someone could change the ProductID on that item after it has been added to _items and passed through our logic. If they do that, for example, we could end up having multiple entries in _items with the same ProductID.

It is the same reason why I try not to expose _items outside of ArrayListCart. If I give people access to the collection, they could start adding, updating, and deleting objects in _items without using the methods on ArrayListCart. IEnumerable gives the developer access to the collection but in a very limited fashion.

I am being very defensive here which could be overkill for many scenarios.

There are other ways to achieve these results. We could have made ProductID read only. This is probably a great idea anyway as it is used to generate GetHashCode() for Item. GetHashCode is not supposed to change during the life of an item and thus ProductID should not either. Right now we are violating that principle by making ProductID a read/write property. More stuff to think about :)

# March 28, 2005 7:38 AM

Thomas Eyde said:

It would be valuable if you could post your requirements with the code. Then it would be easier to see why you add a copy, if Update() really belongs to Item and so on.

# March 29, 2005 2:43 PM