[ 
https://issues.apache.org/jira/browse/OPENNLP-99?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12987591#action_12987591
 ] 

Steven Bethard commented on OPENNLP-99:
---------------------------------------

I guess I don't understand what's unintuitive about the Iterator interface - 
your description of what to do for hasNext() and next() sound perfectly 
sensible to me. I can believe that the implementation might be more difficult 
for Iterator than ObjectStream, but lets put the implementation aside for a bit 
(either one is definitely possible) and talk just about how they would be used.

I have three arguments why you should be using Iterator instead of (or at least 
in addition to) ObjectStream.

(1) Iterator is a Java standard mechanism for iterating over a collection. 
People who have learned Iterators from other Java code can quickly understand 
your code because they already know how Iterators work. ObjectStream is a 
custom interface to OpenNLP and thus few people will have seen it before, thus 
it will take longer to learn.

(2) Using an object with ObjectStream is no easier than using an object with 
Iterator:

// using ObjectStream
Event event = stream.read();
while (event != null) {
  ...
  event = stream.read();
}

// using Iterator
Event event;
while (iterator.hasNext()) {
  event = iterator.next();
  ...
}

It's basically the same amount of code either way. A little more using 
ObjectStream, but it could be a little less if you use an assignment in the 
while loop condition. An added benefit of using the Java standard Iterator 
interface is that you get much better interop with other JVM languages. For 
example, if you use the Iterator interface, you can write the following Scala 
code:

for (event <- iterator) {
  ...
}

If your class implements ObjectStream instead of Iterator, then you have to go 
back to the while loop.

(3) If you use Iterator, then you get Java collections and the enhanced for 
loop for free. For example, you could easily declare a 
RealValueFileEventIterable like this:

public class RealValueFileEventIterable implements Iterable<Event> {
  private File file;
  public RealValueFileEventIterable(File file) {
    this.file = file;
  }
  public Iterator<Event> iterator() {
    return new RealValueFileEventStream(this.file);
  }
}

That's all you need, and then even in Java, people could write:

  for (Event event : new RealValueFileEventIterable(...)) {
    ...
  }

I guess may main argument is that using Iterator makes your code easier to use 
for Java programmers who are familiar with Java standard Interfaces. I think 
making OpenNLP easy to use for general Java programmers is a noble goal.


> EventStream should extend Iterator<Event>
> -----------------------------------------
>
>                 Key: OPENNLP-99
>                 URL: https://issues.apache.org/jira/browse/OPENNLP-99
>             Project: OpenNLP
>          Issue Type: New Feature
>          Components: Maxent
>    Affects Versions: maxent-3.0.0-sourceforge
>            Reporter: Steven Bethard
>
> [As requested, brought over from Sourceforge.]
> Conceptually, EventStream is just an Iterator<Event>. You would get better 
> interoperability with other Java libraries if EventStream were declared as 
> such. If you didn't care about backwards compatibility, I'd say just get rid 
> of EventStream entirely and use Iterator<Event> everywhere instead.
> If you care about backwards compatibility, you could at least declare 
> AbstractEventStream as implementing Iterator<Event> - it declares all of 
> hasNext(), next() and remove(). I believe that shouldn't break anything, and 
> should make all the current EventStream implementations into Iterator<Event>s.
> Why do I want this? Because, when using OpenNLP maxent from Scala, if a 
> RealValueFileEventStream were an Iterator<Event>, I could write:
>   for (event <- stream) {
>     ...
>   }
> But since it's not, I instead have to wrap it in an Iterator:
>   val events = new Iterator[Event] {
>     def hasNext = stream.hasNext
>     def next = stream.next
>   }
>   for (event <- events) {
>     ...
>   }
> Or write the while loop version:
>   while (stream.hasNext) {
>     val event = stream.next
>     ...
>   }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply via email to