search

ACMNexus / google-collections

Automatically exported from code.google.com/p/google-collections
Apache License 2.0
0 stars 0 forks source link

Forwarding collections are easy to misuse #144

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
I had written this on an internal discussion 3 weeks ago

-----8<------
I believe it will be an extremely common mistake when using forwarding
collections to

- override add() but forget to override addAll()
- override iterator() but forget to override toArray(), toArray(),
toString(), hashCode().....

etc.

It's very easy to forget the fact that these collections forward every
method call straight over to the delegate, indiscriminately.  The minimum
solution to this problem is

1. more javadoc warning about this, on the class and individual methods
2. make Collections2 methods public that users can use when they want the
standard self-use patterns instead of straight delegation
or
2-prime: have protected, final methods directly in the ForwardingCollection
classes for this, with names like ... undelegatedAddAll()??  hard to name.
 Then users would do this

    @Override public boolean add(E element) {
      // custom stuff
    }
    @Override public void addAll(Collection<? extends E> elements) {
      undelegatedAddAll(elements);
    }

and the javadoc (mentioned in #1) could refer to these.

If we do that -- and I think I like the idea a lot -- we should sweep
through our own subclasses to see where these could be used.
----->8------

And then the following just appeared independently on our public mailing list:

-----8<------
It seems difficult to know how to use ForwardingList.  How can you
tell which methods you need to override.  It looks like the code just
delegates in all situations which requires you to know how the
delegate is implemented in knowing what to override.  Would it be
better to only delegate a few methods and be able to say in the API
that "all reads go through the ForwardingList#get() and all inserts go
through the ForwardingList#add()".  Then you could easily know you
only had to override one or two methods and wouldn't have to track
down the details of the backing List.

I guess in my situation the only methods that would use the delegate
would be add() and get(:int).

With the general FowardingCollection I suppose you don't have a get()
method but there should be a way to minimize the encapsulation break
points of the delegate.

I would have a hard time telling someone they should use these
classes.  They are filled with peril and can be easily messed up.
There should at least be a disclaimer in the documentation that "using
these classes isn't as easy as you think" and highlight some of the
break points because I can see a lot of people having a heck of a time
with very tricky bugs.

I understand that they can be used effectively by but I worry they
make things seem too simple for and entice those that are less
vigilant.
----->8------

Original issue reported on code.google.com by kevin...@gmail.com on 13 Apr 2009 at 2:42

GoogleCodeExporter commented 8 years ago

Original comment by kevin...@gmail.com on 14 Apr 2009 at 3:48

GoogleCodeExporter commented 8 years ago

Original comment by kevin...@gmail.com on 17 Sep 2009 at 5:45

GoogleCodeExporter commented 8 years ago

Original comment by kevin...@gmail.com on 17 Sep 2009 at 5:46

GoogleCodeExporter commented 8 years ago

Original comment by kevin...@gmail.com on 17 Sep 2009 at 5:59

GoogleCodeExporter commented 8 years ago

Original comment by kevin...@gmail.com on 16 Oct 2009 at 9:02

GoogleCodeExporter commented 8 years ago 
This issue has been moved to the Guava project (keeping the same id number). 
Simply replace 'google-collections' with 'guava-libraries' in your address 
bar and it should take you there.

Original comment by kevinb@google.com on 5 Jan 2010 at 11:09