A design decision in composition or aggregation

Question

I've recently had doubts about some design decisions that I frequently make, so this time I decided to take some time and try to analyze it to find the best solution. I will describe a simple scenario where the design decision can be easily understood. :)

Suppose we are developing a game in Java where players can have any number of many different types of items.

/** Class that represent an item type existing in the game */
// you may prefer to call this ItemType,
// but for me appending the word "Type" is not necessary
class Item {
    private String descrip;
    private Image icon;
}

In this situation I need to keep track of how many of each item the player has in his bag, so I keep a collection of them, in each element of which I have item type and quantity. Additionally one can think of other attributes that belong to that relation (perhaps a max capacity), but the question is: how do I model this scenario in general but with a practical implementation in Java? Here are two approaches I've come up with:

Using a List

class Slot {
    private Item item;
    private int quantity;

    /** Auto generated */
    public int hashCode()
    {
        int hash = 7;
        hash = 13 * hash + (this.item != null ? this.item.hashCode() : 0);
        return hash;
    }

    /** This method is also auto generated and just compares the Item, 
        independent of quantity */
    public boolean equals(Object object) 
    {
        if (!(object instanceof Slot))
        {
            return false;
        }
        Slot other = (Slot) object;
        if ((this.item == null && other.item != null) 
             || (this.item != null && !this.item.equals(other.item)))
        {
            return false;
        }

        return true;
    }
}

The above class relates one item with a quantity, and a collection of Slots is the primary attribute of the Inventory class. The disadvantages of this appear as soon I design the Inventory interface to not expose Slot to the client (making Slot private or hiding it in some other way): in creating a simple interface but providing access to all the standard collection methods. In my humble opinion there is nothing wrong with this:

/** Class that represent the bag that holds the player's items */
class Inventory {
   // this collection holds only one element for each type of item
   private ArrayList<Slot> slots; 

    public Inventory()
    {
        slots = new ArrayList<Slot>(10);
    }

    public void add(Item item, int quantity)
    {
        Slot slot = new Slot(item, quantity);
        // if we don't already have a Slot for the added Item, add it
        if (!slots.contains(slot))
        {
            slots.addElement(slot);
        }
        // if we already have a Slot for the added Item, increase the quantity
        else
        {
            Slot _slot = slots.elementAt(slots.indexOf(slot));
            _slot.addQuantity(quantity);
        }
    }

    public void subtract(Item item, int quantity)
    {
        Slot slot = new Slot(item, 0); 
        int index = slots.indexOf(slot);

        // only if we actually have this item
        if (index != -1) 
        {
            Slot _slot = (Slot) slots.elementAt(index);
            _slot.quantity -= quantity;

            // if we have fewer of the Item than the number to remove, 
            // remove the slot from the collection
            if (_slot.quantity <= 0)
            {
                slots.removeElementAt(index);
            }
        }
    }

    public void remove(Item item)
    {
        Slot slot = new Slot(item,0);
        slots.removeElement(slot);
    }

    public boolean hasItem(Item item)
    {
        Slot slot = new Slot(item,0);
        return slots.contains(slot);
    }
}

As you see, we have to jump through some hoops to implement certain collection methods: in hasItem we have to create a Slot element just for that operation. In the add and subtract methods, we have to create the Slot object just to check if it's there. I wonder if this really necessary. I suppose that it is if we desire to provide an interface to the standard collection methods. Perhaps we can move all this logic to another class (for example: Slots), exposing a cleaner interface that lets us search by item without the need of creating a Slot, but we are just moving the same problem to another class. We will face the same problem there, besides the fact that Inventory is basically a set of Slots, we're really just creating another class to be exactly the same thing, which doesn't make a lot of sense to me.

Would it be more correct to change the interface of this class to work directly with Slot class instead? This implies moving from composition (having Slots created within Inventory) to an aggregation (having Slot creation outside of Inventory). The result might look like:

/** Class that represent the bag that holds the player's items */
class Inventory {
   private ArrayList<Slot> slots;

    public Inventory()
    {
        slots = new ArrayList<Slot>(10);
    }

    public void add(Slot slot)
    {
        if (!slots.contains(slot))
        {
            slots.addElement(slot);
        }
        else
        {
            Slot _slot = slots.elementAt(slots.indexOf(slot));
            _slot.addQuantity(slot.getQuantity());
        }
    }

    public void subtract(Slot slot, int quantity) // how would the client get the Slot?
    {
        int index = slots.indexOf(slot);

        // only if we actually have this item
        if (index != -1) 
        {
            Slot _slot = (Slot) slots.elementAt(index);
            _slot.quantity -= quantity;

            // if we have fewer of the Item than the number to remove, 
            // remove the slot from the collection
            if (_slot.quantity <= 0)
            {
                slots.removeElementAt(index);
            }
        }
    }

    public void remove(Slot slot) // again, how would the client get the Slot?
    {
        slots.removeElement(slot);
    }

    // Hard to change a parameter type here. This strongly suggest there is
    // a flaw in the design. I only keep this here to communicate the observation
    public boolean hasItem(Item item) 
    {
        Slot slot = new Slot(item, 0); // here we have no choice but to create it
        return slots.contains(slot);
    }
}

But again, is this convenient? I don't often put this approach into practice, so I'm not sure.

Using a lookup

Another approach is to use something like a Hashtable with Item objects as keys quantities as values, so only quantity is associated with Items, without the need for a Slot class. An Inventory implementation using Hashtable might look like:

public class Inventory
{
    private Hashtable items;

    public Inventory()
    {
        items = new Hashtable(10);
    }

    public void add(Item item, int amount)
    {
        int stock = amount;
        if (items.containsKey(item))
        {
            stock += ((Integer) items.get(item)).intValue();
        }
        items.put(item, new Integer(stock));
    }

    public void subtract(Item item, int amount)
    {
        if (items.containsKey(item))
        {
            int stock = ((Integer) items.get(item)).intValue();
            stock -= amount;
            if (stock > 0)
            {
                items.put(item, new Integer(stock));
            }
            else
            {
                items.remove(item);
            }
        }
    }

    public void remove(Item item)
    {
        this.items.remove(item);
    }

    public boolean hasItem(Item item)
    {
        return this.items.containsKey(item);
    }
}

To me this seems much better than using a List because there is no need of the Slot class, and all interactions with the Item collection are much simpler, but I'm not sure if I'm right.

The same type of problem is also evident in this scenario: Suppose that we are implementing an order application for a pizza shop; every order consists of types of pizzas and quantities.

/** Type of pizza */
class Pizza {
    private String descrip;
    private Image icon;
}

/** Customer request for pizza */
class Request {
    // for each type of pizza we have to keep a count of how many the customer ordered
}

/** A pizza order */
class Order {
    private Pizza pizza;
    private int   amount;
    privete BigDecimal price;
}

Knowing how to solve the first problem, I have a pattern to use for this similar situation; so the question is, what solution is best?

I try to convince myself that I'm not the only person with this concern, that ordinary people like me face this problem on a regular basis, so I hope to find how other people solve this problem. On the other hand, I know that this question is quite big and maybe a little elaborate, so maybe I need to split it into several more specific questions. Maybe the answer to this problem is simple I'm over-complicating things, or even worse, my doubts about how to handle this situation arise because I really don't understand some basic design principle, like a captain on a ship using a spyglass in order to sight land but doesn't realize that his ship has been beached on solid ground that was close at the moment he draw his spyglass to look. Okay, enough cheap poetry (but it was hard to describe my feelings without it hehehe).

I apologize for the length of the question, and would very much appreciate anyone's help with this problem.

Answer 1

There's a lot to say here.

1. Inventory's api looks like a value object (since it has no identity, on its own, I suppose that it belongs to a Player). If so, it should be immutable, returning a new instance each time you add or remove an Item.
2. Your Item class looks like a shared identifier, thus it should override equals and hashcode and (strictly speaking, in DDD term) it should not hold any reference to the icon since it's not useful for its own invariants. You should use a domain service that take the Item and provide the proper icon instead (probably wrapping a statically defined Hashtable). However, be pragmatic with this: if you measure actual performance issues with such (formally correct) approach, keep the field there.
3. The Slot, as it stands, cannot be considered a domain object, because it does not implement equality in a domain relevant way (it ignores the quantity). This is perfecly correct for a private class internally used by the Inventory, but should not be exposed to clients (as in the second Inventory's implementation of the first solution you proposed). This because the clients could compare for equality two Slots from two different players, obtaining unreliable results.
4. To get rid of most boilerplate code, I would code
   • a supporting functor interface like ItemQuantityChanger (below)
   • a "functional" immutable collection, say ItemsSet, with a metod of signature ItemsSet set(Item item, ItemQuantityChanger transformation) that iterates over a simple array for the item to change. Such method would always create a new ItemsSet to return, without the Item (if the trasformation returns zero), with a new Item if the item is not found (with the quantity returned by applying the transformation to zero) or simply with the previous item coupled with the result of the trasformation.

However, concretely, I would just use an ImmutableList.Builder, since the functional design described in 4 adds few value in this case. A single if, in a method, is not that huge smell.

public interface ItemQuantityChanger {
   /**
    * Accept the current quantity of an Item and return the new one.
    */
   int applyTo(int currentQuantity);
}

Answer 2

As far as I can tell, your question boils down to (in the context of your example):

1. What kind of interface should Inventory have?

2. How should Inventory track its Item collection?

What kind of interface should Inventory have?

Inventory is an abstraction that simplifies how the Player class deals with its items, so the interface should be defined so that Player knows nothing about the implementation of that collection. This allows you to completely change how Inventory tracks items - maybe you want to start tracking total capacity by Item weight instead of size, etc - without requiring any changes in the Player code. Besides that, managing the collection of Items is the responsibility of Inventory, so the logic to do so doesn't belong anywhere else.

Just a note: max capacity doesn't seem to belong where you described it. Ideally your Inventory would have a max capacity, say 100, and different Items would contribue a certain value to that capacity; for example, a health potion would contribute 5 units, a pair of boots would contribute 20 units, etc.

How should Inventory track its Item collection?

A List is preferable if you want to hold multiple copies of the same item, and since in this case you want only distinct items, use a lookup. Hashtable is synchronized, but naively (see one of the comments to this SO question), so HashMap would probably serve better. Of course since we should always program to interfaces rather than implementations, it would be:

Map<Item, Integer> items = new HashMap<Item, Integer>();

Another note: you don't generally need to specify initial collection capacity unless you're optimizing, and in any case, Hashtable's initial capacity is 11, and HashMap's is 16.

---

To get a better handle on design principles, read up on SOLID principles, and if you want to go in-depth in all aspects of software development, buy a copy of Code Complete, and read it. :-)