search

kgislsompo / guava-libraries

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

Forwarding collections are easy to misuse #144

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 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 9 years ago

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

GoogleCodeExporter commented 9 years ago

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

GoogleCodeExporter commented 9 years ago

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

GoogleCodeExporter commented 9 years ago

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

GoogleCodeExporter commented 9 years ago

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

GoogleCodeExporter commented 9 years ago 
I don't see this much as a problem. Users have to know (!) the specification 
they are forwarding. Everybody who overwrites add() to ensure every add takes 
places in that method should read the spec carefully.

Original comment by w.schoen...@gmail.com on 14 Jul 2010 at 3:42

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 30 Jul 2010 at 3:53

GoogleCodeExporter commented 9 years ago 
These methods are in the process of being added for release 7, in @Beta.

Original comment by kevinb@google.com on 16 Sep 2010 at 10:41

GoogleCodeExporter commented 9 years ago 
Most of these are added now and in release 7, but some like the Multimaps had 
to wait.

Original comment by kevinb@google.com on 22 Sep 2010 at 9:38

GoogleCodeExporter commented 9 years ago 
Is there any particular reason e.g. the Multimaps didn't make it into release 
8?  (I seem to recall you have the code... ;) )

Original comment by wasserman.louis on 2 Feb 2011 at 6:11

GoogleCodeExporter commented 9 years ago 
I think we passed the point of diminishing returns somewhere along the way, and 
it's hard to justify spending more time on it... but this issue remains open 
for now.

Original comment by kevinb@google.com on 3 Feb 2011 at 3:00

GoogleCodeExporter commented 9 years ago 
I don't think it's worth going further at this point.

Original comment by kevinb@google.com on 8 Apr 2011 at 2:12

GoogleCodeExporter commented 9 years ago

Original comment by cpov...@google.com on 1 Sep 2011 at 9:23

GoogleCodeExporter commented 9 years ago 
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:16

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:10