AbstractIterator shouldn't extend UnmodifiableIterator

[GoogleCodeExporter]

All is in the title.

I don't understand why, since r80 (and so 1.0-rc1), an iterator based upon
"AbstractIterator" may not override the method remove anymore. Providing
the default behavior throwing an exception is good, because most iterators
don't actually modify the collection (or whatever) they are based on, but
preventing every child from this possibility is non-sense to me: I don't
see any reason why AbstractIterator now extends UnmodifiableIterator, even
with Effective Java on my side.

There must be some reason, but I don't see any.

Original issue reported on code.google.com by ogregoire on 21 Aug 2009 at 11:09

[GoogleCodeExporter]

Whenever library methods supply an iterator that doesn't support remove(), we 
wanted 
to return an UnmodifiableIterator. That way, the method signature tells you 
that the 
iterator isn't modifiable, which is better than stating that fact in a separate 
sentence of the Javadoc. Since many of those iterators are subclasses of 
AbstractIterator, our best option was to make AbstractIterator extend 
UnmodifiableIterator.

Besides, there's a straightforward workaround when you want remove() support. 
Define 
a delegate iterator that extends AbstractIterator, and create a 
ForwardingIterator 
that overrides remove().

Original comment by jared.l....@gmail.com on 21 Aug 2009 at 2:00

• Changed state: WontFix

[GoogleCodeExporter]

Why then not make UnmodifiableIterator extend AbstractIterator instead of the
opposite ? I think this is a real design flaw because people, when they see
Abstract*, expect to be able to modify any method, or have some way to get the
skeletal implementation work as the interface allows with optional operation 
still
optional. This current implementation somehow breaks the Effective Java II item 
18
(p. 95, first paragraph).

Original comment by ogregoire on 21 Aug 2009 at 2:26

[GoogleCodeExporter]

Additionally, we had an AbstractIterator that supported remove() internally.  
In over
a year, it ended up with one user outside the package, and that user's use of 
it had
a subtle bug that had gone undetected despite unit testing.  We felt that this 
was a
sign that the class was more likely to be misused than used correctly.

Original comment by cpov...@google.com on 21 Aug 2009 at 3:01

[GoogleCodeExporter]

If you could override remove(), it would be very hard to do it correctly, and 
easy to
introduce a bug.  As Chris pointed out, even our removable version of the class 
was
being misused more than it was used properly.  Sorry... AbstractIterator still 
covers
the lion's share of use cases and that's what mattered to us.

Original comment by kev...@google.com on 21 Aug 2009 at 3:09

[GoogleCodeExporter]

I fully agree: AbstractIterator covers 95% of use-cases, and the remove() 
method is
not used very much... but still it is sometimes used. Then why not documenting 
the
class on how to correctly implement the remove() method, and warn about the 
possible
misuse (which is even not in AbstractIterator, but was in 
AbstractRemovableIterator
and, if you allow me, was as big as a house) ?

By switching UnmodifiableIterator and AbstractIterator, there is indeed some 
work to
perform, but you still allow an Iterator to be used as it should be working.

Another solution would be to create AbstractUnmodifiableIterator (that you would
recommend in most cases) and leave the door open to AbstractIterator with 
remove().
Here also the workload doesn't seem big. It is even lower than the previous 
solution.

Furthermore, I don't ask to come back to AbstractRemovableIterator: simply to 
allow
the override of remove(), leaving the default behavior that throws an UOE.

Finally, I do see your point, but I don't understand why documenting seems to 
be so
problematic.

Original comment by ogregoire on 21 Aug 2009 at 4:56

[GoogleCodeExporter]

It wouldn't hurt to provide two examples of how you'd personally override 
remove() in
an AbstractIterator.

Original comment by kevin...@gmail.com on 21 Aug 2009 at 5:04

[GoogleCodeExporter]

We can't make UnmodifiableIterator extend AbstractIterator, since some 
unmodifiable
iterators don't use the AbstractIterator implementation logic.

My Javadoc reasoning reflected a broader concern about API design. It's better 
for a
method to return a more specific type when the additional type info is helpful. 
For
example, if a method returns a collection without any duplicates, it's better to
return a Set than to return a Collection and have Javadoc saying that the 
elements
are unique.

That design principle is more important than providing support for an unusual 
use
case, especially since a workaround exists for that use case.

Original comment by jared.l....@gmail.com on 21 Aug 2009 at 7:59

[GoogleCodeExporter]

Okay with that. So why not make a distinction between AbstractIterator and
AbstractUnmodifiableIterator (or AbstractRemovableIterator), so people who 
actually
want to override remove() for clear reason may do so?

I'm currently not very in the mood to create the two requested examples, but 
they
will be done by the end of the week-end.

Original comment by ogregoire on 21 Aug 2009 at 9:14

[GoogleCodeExporter]

I'd like to add that the design of AbstractIterator is more in favor of a 
implementing a generator: an iterator that calculates its items on the fly. 
remove() 
doesn't apply to a generator.

Although i personally do not really like the current implementation. Calling 
endOfData() and then returning an irrelevant null value doesn't play nice. :)

For the purpose of a generator, I'm more in favor of a .NET style iterator with 
a 
'boolean moveNext()' and a 'T current()'.

AbstracIterator also supports peek() but doesn't implement PeekingIterator

Original comment by jvdne...@gmail.com on 26 Aug 2009 at 9:53