Search code examples
javacollectionsdecoratorguavageneric-collections

Unexpected behaviour decorating a collection using Guava ForwardingList

I've encountered a very nasty i would say 'side effect', but it's obviously a problem of bad design. I'm using Guava ForwardingList pattern to decorate a regular List. My purpose is to build a size-limiting List where oldest elements are kicked out when a maximumSize is met(a simple FIFO design). Note that i don't wont to proxy or clone my existing collection. But i have this very nasty side effect:

List<String> originalList = new ArrayList<String>();
int maximumSize = 2;

originalList.add("foo");
originalList.add("bar");

System.out.println(originalList); // [foo, bar]

ListFactory<String> factory = ListFactory.getInstance(String.class);
List<String> decoratedList = factory.newTalendList(originalList, maximumSize);

decoratedList.add("beer");
System.out.println(originalList); // [bar, beer]

originalList.add("ben");
System.out.println(originalList); // [bar, beer, ben] <-- !!!
System.out.println(decoratedList); // [bar, beer, ben] <-- !!!

(note: my decorated class overrides add() to remove the first element of the list upon adding a new one. All other non-overridden methods, included toString(), are delegated to original List)

Ok, as you may see if i add an element using the original add() method i can exceed the maximumsize...ok, i suppose it's unavoidable (and that's not wrong by design, after all). But this is not true-by-design for decoratedList.

The only workaround i found was something like:

List<String> decoratedList = factory.newTalendList(new ArrayList<String>(originalList), maximumSize);

but it doesn't seems the best way (and i'm not sure it works in every circumnstances): i'm not decorating originalList, but her anonymous clone! I'm wondering: perhaps have I completely messed with my design? Is there a better way to build it?

Solution

  • The only way this pattern is going to work is if you never reference originalList again after you've created the decorator. The ForwardingList couldn't possibly have any control over what happens in the originalList. (No decorator could.)

    What you should probably do, overall, is to create a factory method that returns a completely new decorated list and never lets you access the original list at all.