Hi Marko,
great observations. More comments inline:
On 24 September 2012 13:30, Marko Lukša <marko.luksa(a)gmail.com> wrote:
Hey
Infinispan's implementations of QueryIterator don't conform to the
contract of ListIterator and are pretty buggy (see
https://issues.jboss.org/browse/ISPN-2337). The most important bug is
the fact that calling next() and then previous() should return the same
element, but it doesn't.
+1, that's wrong, fix is very welcome.
Funny, one of my first contributions to Hibernate Search was rewriting
the Scrollable implementation to fix a similar problem.
I'm trying to fix the bugs, but have now realized that even the
interface QueryIterator is problematic.
1. QueryIterator.first()
This one isn't as problematic as the next ones, but it's strange that
calling first() and then next() returns the first element of the
collection. It would be clearer if the method were called
"beforeFirst()" or something similar.
+1 for "beforeFirst()"
(Hibernate's Scrollable uses the same name)
2. QueryIterator.last()
What should the iterator return when you call last() and then call
next()? What about if you call last() and then previous()? A user would
probably call last() as the first step of iterating over the results
backwards. So calling previous() should return the last element. The
method should probably be renamed to "afterLast()".
+1 for the rename
3. QueryIterator.afterFirst() & beforeLast()
These two are OK. Calling next() after afterFirst() should return the
second element, while calling previous() should return the first element.
Are you sure they are ok? I'm actually wondering why they exist at
all; they don't look like very useful to me.
Why should someone want a method to directly jump to the second element?
Users already have the option to "beforeFirst && next" or use the
"jumpTo" / seek operation.
4. isFirst(), isLast()
ListIterator does not have the concept of "current element", but the
javadoc of all the is*() methods talk about the current element, which
is ambiguous. Therefore isFirst() and isLast() should probably be
renamed to isBeforeFirst() and isAfterLast().
I would remove these too. Am I missing some use case?
5. jumpToResult(int index)
When jumping to index=1, what does next() return? What about previous()?
We should probably replace the method with two others: beforeResult(int
index) and afterResult(int index)
I would add only "beforeResult(int)", again I don't see why we should
have both versions.
Since you have to do "next()" to retrieve a value, I'd consider
"beforeResults" more user friendly than "afterResults"
Any comments / other ideas?
Marko
That's a lot of changes needed. One would say we could deprecate all
methods and rewrite the new ones, but wouldn't it be better to
deprecate the interface and introduce a brand new one?
What about "ResultsIterator" ? after all, we're not iterating on queries..
Sanne