Hi Marko, great observations. More comments inline: On 24 September 2012 13:30, Marko Lukša <[email protected]> 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 _______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
