Using a Composite class: how a client can determine whether it is composite

Question

I am aware that there is a similar question to this here. It considered a more generic question of class specific behaviour than my question here.

Consider the following simple implementation of a Composite pattern:

interface Item {
    int getWeight();
}

class SimpleItem implements Item {
    private int weight;
    public int getWeight() {
        return weight;
    }
}

class Container implements Item {
    private List<Item> items;
    private int weight;
    public void add(Item item) {
        items.add(item);
    }
    public int getWeight() {
        return weight + items.stream().mapToInt(Item::getWeight).sum();
    }
}

Now consider how a user of Item can determine whether it is a container. For example a method is required in Container pushAdd that pushes an item down the hierarchy to a container that has no containers inside it. The container only knows about Items, it doesn't know if those items are Containers or SimpleItems or some other class that implements Item.

There are three potential solutions:

1. Using instance of and casting

public void pushAdd(Item item) {
    Optional<Container> childContainer = items.stream()
        .filter(item instanceof Container)
        .map(item -> (Container)item)
        .findAny();
    if (childContainer.isPresent()) {
        childContainer.get().pushAdd(item);
    } else {
        add(item);
    }
}

2. Implement is/as methods

public pushAdd(Item item) {
    Optional<Container> childContainer = items.stream()
        .filter(Item::isContainer)
        .map(Item::asContainer);
    ....
}

3. Visitor pattern (I've omitted the simple accept implementation).

interface ItemVisitor {
    default void visit(SimpleItem simpleItem) { throw ...}
    default void visit(Container container) { throw ... };
}

public pushAdd(Item item) {
    Optional<Container> childContainer = ... (using instanceOf);
    if (childContainer.isPresent()) {
        childContainer.get().accept(new ItemVisitor(item) {
            void visit(Container container) {
                container.pushAdd(item);
            }
        };
    } else {
        add(item);
    }
}

The first is evil because it uses instanceof and casting. The second is evil because it forces knowledge of Container into Item - and it gets a lot worse when other subclasses of item are created. The third doesn't help you know whether you can add to the Item before calling the visitor. You can catch the exception but that seems to be a misuse of exceptions to me: much better to have a way to check before visiting.

So my question is: is there another pattern I can use to avoid casts and instanceof without having to push knowledge of subclasses up the hierarchy?

Answer 1

I think I speak for any java person when I say that the visitor pattern is not exactly popular in java. So the above could be implemented like this (I will use interfaces here because in my experience they are more flexible):

interface Item { /* methods omitted */ }

interface SimpleItem extends Item { /* methods omitted */ }

interface ContainerItem extends Item { /* methods omitted */ }

// telling clients that this can throw an exception or not
// here is a whole different design question :)
interface ItemVisitor { void visit(Item i) /* throws Exception */; }

class MyVisitor implements ItemVisitor {
  void visit(Item i) {
    if (i instanceof SimpleItem) {
      // handle simple items
    } else if (i instanceof ContainerItem) {
      // handle containers using container specific methods
    } else {
      // either throw or ignore, depending on the specifications
    }
  }
}

The cost of instanceof is pretty low on the latest JVM so I would not worry about that too much, unless you can prove that the traditional visitor is considerably faster.

The readability and maintainability of the code is arguably identical, with a few differences. First of all, if a new interface is added to the hierarchy that does not modify existing visitors, those visitors don't need to be changed. On the other hand, it's easier to overlook a visitor that does need to be changed (especially in client code that you don't control) because the visitor does not explicitly require clients to do so, but, hey that's the nature of maintaining code and the general shortcoming of a design that requires visiting.

Another advantage of this pattern is that clients that don't need visiting don't need to worry about it (there's no accept method), i.e., a shorter learning curve.

Finally, I think this pattern is closer to the "purist" OOD in the sense that the interface hierarchy does not contain spurious methods (visit, canAddItems, etc.), i.e., there are no "tags" of sorts.

Answer 2

Well it seems from the fact that no one has posted an answer that there are no better options than the 3 I put up.

So I'll post my preferred solution and see if anyone can improve on it. In my view the best option is a combination of options 2 and 3. I don't think it's too evil to have a canAddItems member of Item - it's arguable that it's reasonable that implementers of Item tell you if you can add Items to them. But visitors seem like a good way of hiding the details of how to items are added.

So, fwiw this is my best compromise to the problem I posed. I'm still not 100% happy with it. In particular the visitor to add items will break if another class is implemented that can add items. But that's probably what you want because it's changing the semantics of pushAdd.

interface Item {

    int getWeight();

    void accept(ItemVisitor visitor);

    default boolean canAddItems() {
        return false;
    }

}

interface ItemVisitor {

    default void visit(SimpleItem simpleItem) {
        throw new IllegalArgumentException("ItemVisitor does not accept SimpleItem");
    }

    default void visit(Container container) {
        throw new IllegalArgumentException("ItemVisitor does not accept Container");
    }
}

class SimpleItem implements Item {

    private int weight;

    public int getWeight() {
        return weight;
    }

    public void accept(ItemVisitor visitor) {
        visitor.visit(this);
    }
}

class Container implements Item {

    private List<Item> items;
    private int weight;

    public void add(Item item) {
        items.add(item);
    }

    public int getWeight() {
        return weight + items.stream().mapToInt(Item::getWeight).sum();
    }

    public void accept(ItemVisitor visitor) {
        visitor.visit(this);
    }

    public void pushAdd(Item item) {
        Optional<Item> child = items.stream().filter(Item::canAddItems).findAny();
        if (child.isPresent()) {
            child.get().accept(new ItemVisitor() {
                public void visit(Container container) {
                    container.add(item);
                }
            });
        } else {
            add(item);
        }
    }

}