Request: Enhance the Forwarding* classes to actually "hold" the decorated state

[GoogleCodeExporter]

In almost all use cases the delegate() methods of the Forwarding*
subclasses will want to return the same state preserving instances on each
call. This is especially true for the *Iterator, where the delegate holds
the iteration state. However, in all those cases the subclass is
responsible for catering that same instance, which almost always comes from
a member in that subclass or from a final local in a surrounding block (in
case of an anonymous subclass). This adds useless boiler-plate code, which
may be error prone (at least, in the *Iterator case; see below).

I propose to add another constructor to the Forwarding* classes which takes
the delegate instance as an argument and stores it in a member. The default
delegate() method would then return that member. The no-argument
constructor would delegate to the one-argument constructor, passing it the
static final STOPGAP instance, which simply throws meaningful exceptions on
every call to the implemented interface to inform the user to either pass
in a valid delegate on the constructor call or to override delegate() in
the subclass.

Suppose something like this for the *Iterator:

abstract public class ForwardingIterator<T> {
  final private Iterator<T> delegate;
  protected ForwardingIterator() { this(STOPGAP); }
  protected ForwardingIterator(Iterator<T> delegate) {
    this.delegate = checkNotNull(delegate);
  }

  protected Iterator<T> delegate() { return this.delegate; }

  /* throws an exception on every call to the iterator interface indicating
to "pass a delegate to the constructor" or to "override delegate()". */
  protected final static Iterator<T> STOPGAP = ... ;
}

Then, instead of doing this (which would still be possible):

Iterator<T> iterator() {
  final Iterator<T> delegate = newDelegate();
  return new ForwardingIterator<T>() {
    protected Iterator<T> delegate() {
      return delegate;
    }
    ...
  }
}

one could simply do this:

Iterator<T> iterator() {
  return new ForwardingIterator<T>(newDelegate()) {
    ...
  }
}

as opposed to this (very wrong) code:

Iterator<T> iterator() {
  return new ForwardingIterator() {
    protected Iterator<T> delegate() {
      return newDelegate(); // resets the iteration state
    }
    ...
  }
}

Original issue reported on code.google.com by Robin...@gmail.com on 17 Aug 2009 at 6:07

[GoogleCodeExporter]

Thanks for the suggestion.

Initially, the Forwarding classes did contain the delegate. However, that 
approach
had one serious drawback: it caused problems with serialization.

Serialization of a subclass is easier when the superclass has a zero-argument
constructor and lacks internal state that needs to be persisted. In addition, 
with
the current design, you can define a subclass that doesn't implement 
Serializable,
for cases when your class shouldn't be serialized.

Those serialization concerns outweigh the factors that you mentioned.

Original comment by jared.l....@gmail.com on 17 Aug 2009 at 6:21

• Changed state: WontFix

[GoogleCodeExporter]

Robin, can you make your own subclasses of the Forwarding base classes to have 
the 
behavior you desire?

Original comment by kevin...@gmail.com on 17 Aug 2009 at 6:28

[GoogleCodeExporter]

Note this alternative approach:

public static <T> ForwardingIterator<T> create(final Iterator<T> delegate) {
  return new ForwardingIterator<T>() {
    @Override protected Iterator<T> delegate() {
      return delegate;
    }
  };
}

This approach breaks serialization only for those who opt in, and it can be 
written
without modifying ForwardingIterator itself.  The distinction between creating a
subclass manually and using the create() method could be confusing, but does 
this
sound like an acceptable tradeoff, Jared?

Original comment by cpov...@google.com on 17 Aug 2009 at 6:33

[GoogleCodeExporter]

In the majority of the cases I'm talking about, the delegate instance will be 
there
anyway as a part of the internal state -- either as an explicit user-defined 
member
or as a compiler-indroduced synthetic member. Which ever it is, the user has to 
cater
for the serializability on his own anyway.

I'm not so much concerned with serialization... maybe I don't see it.

Original comment by Robin...@gmail.com on 17 Aug 2009 at 6:45

[GoogleCodeExporter]

@kevinb9n: I could certainly implement my own subclass, but what's the use of a
collection framework then?

Original comment by Robin...@gmail.com on 17 Aug 2009 at 6:47

[GoogleCodeExporter]

That's a reasonable idea, with one change.

Instead of ForwardingIterator.create(), it would be better to have a
Forwarders.createIterator() method. That avoids subtle problems arise when, for
example, ForwardingCollection and its subclass ForwardingSet have static 
create()
methods with different return types.

Original comment by jared.l....@gmail.com on 17 Aug 2009 at 6:59

[GoogleCodeExporter]

Oh, but this ignores the primary purpose of ForwardingIterator: to allow you to
override methods.  So scratch that.

Original comment by cpov...@google.com on 17 Aug 2009 at 7:03

[GoogleCodeExporter]

"what's the use of a collection framework then?"

If it's of no use to you in your case, then simply don't use it!

Original comment by kevin...@gmail.com on 17 Aug 2009 at 7:13

[GoogleCodeExporter]

Robin, here are a couple of quotes from Effective Java regarding serialization:

p. 174   "Classes designed for inheritance (Item 17) should rarely implement
Serializable, and interfaces should rarely extend it."
p. 175   "Therefore, you should consider providing a parameterless constructor 
on
nonserializable classes designed for inheritance."

You can read the book for more details, which agree with my experience while 
cleaning
up the library's serialization logic.

The library could include classes like NonSerializableForwardingIterator that 
behave
the way you want. However, that approach would bloat the library with 14 
additional
public classes.

Original comment by jared.l....@gmail.com on 17 Aug 2009 at 8:23

[GoogleCodeExporter]

> "what's the use of a collection framework then?"
>
> If it's of no use to you in your case, then simply don't use it!

Don't be so bitchy, my request was just a suggestion, which obviously "won't 
fix".

@jared.l.levy: Unfortunately, Josh's Effective Java is not among my books, yet.
Anyways, I see your concerns, and that you've probably already discussed this 
matter
earlier. Don't bother about this any further. I will simply implement my own
("optionally serializable") classes and maybe provide you with the result later 
on.

Original comment by Robin...@gmail.com on 18 Aug 2009 at 9:04

[GoogleCodeExporter]

I'm sorry for any bitchy implications.  I was speaking quite literally, though. 
 The
purpose of a convenience API is to be, um, convenient for a certain set of
circumstances, and the fact that a circumstance exists for which the API is not
convenient does not invalidate the entire library.  So, use these things when it
helps you, and please don't use them when they don't.

Original comment by kevin...@gmail.com on 18 Aug 2009 at 5:09