Commons Collections
  1. Commons Collections
  2. COLLECTIONS-471

Add a BoundedIterator which returns at most limit elements

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1
    • Component/s: None
    • Labels:
      None

      Description

      It would be nice to have a decorator which bounds the number of elements returned by the decorated iterator.

      1. BoundedIteratorTest.java
        12 kB
        Radford Tam
      2. BoundedIterator.java
        6 kB
        Radford Tam

        Activity

        Hide
        Thomas Neidhart added a comment -

        The reason would be consistency. Normally all decorators, see also other packages, have a static factory method.
        But I have to admit that some things are not consistent:

        • some decorators have a private constructor, mainly when they are supposed to be final, e.g. unmodifiable
        • some have a protected constructor to allow sub-classing
        • some have a public constructor that does not validate the input

        This needs to be further cleaned up in a reasonable way. For iterators I am not yet decided, and another thing to consider is to shift our focus more on Iterables rather than on Iterators, see COLLECTIONS-442, which I want to tackle next.

        Show
        Thomas Neidhart added a comment - The reason would be consistency. Normally all decorators, see also other packages, have a static factory method. But I have to admit that some things are not consistent: some decorators have a private constructor, mainly when they are supposed to be final, e.g. unmodifiable some have a protected constructor to allow sub-classing some have a public constructor that does not validate the input This needs to be further cleaned up in a reasonable way. For iterators I am not yet decided, and another thing to consider is to shift our focus more on Iterables rather than on Iterators, see COLLECTIONS-442 , which I want to tackle next.
        Hide
        Radford Tam added a comment -

        Is there some advantages to having those factory methods? The class has public constructors. And I see that mostly it's the EmptyIterators and the UnmodifiableIterators that have them, which make sense for them because of how they function.

        To be honest, after reviewing the code, it seems the factory methods are unnecessary, like the getIterator method.

        Show
        Radford Tam added a comment - Is there some advantages to having those factory methods? The class has public constructors. And I see that mostly it's the EmptyIterators and the UnmodifiableIterators that have them, which make sense for them because of how they function. To be honest, after reviewing the code, it seems the factory methods are unnecessary, like the getIterator method.
        Hide
        Thomas Neidhart added a comment -

        Good question.

        The AbstractIteratorDecorator defines a getIterator method, which is protected, thus only intended to be used by derived classes, not by users. This is similar to the collection-type decorators, where the corresponding decorated() method is usually also protected.

        For our iterator decorators it is less likely that somebody will extend them, so there is less of a need to provide access to the underlying iterator, and I fail to see a use-case where a user decorates an iterator and later on wants to retrieve the decorated iterator again (and would rather design the API in a way to prevent such usage).

        That said, there are now some iterator decorators, like Unmodifiable that provide access to the decorated iterator, and some of the newer ones, like PeekingIterator, that dont. I would prefer to not provide access at all and deprecate the existing getIterator() methods for future versions. There are some weird things in the API that were added a long time ago, and I did not want to change everything when doing the 4.0 release, but we can decide to gradually improve the API and deprecate things that do not make sense anymore.

        Show
        Thomas Neidhart added a comment - Good question. The AbstractIteratorDecorator defines a getIterator method, which is protected, thus only intended to be used by derived classes, not by users. This is similar to the collection-type decorators, where the corresponding decorated() method is usually also protected. For our iterator decorators it is less likely that somebody will extend them, so there is less of a need to provide access to the underlying iterator, and I fail to see a use-case where a user decorates an iterator and later on wants to retrieve the decorated iterator again (and would rather design the API in a way to prevent such usage). That said, there are now some iterator decorators, like Unmodifiable that provide access to the decorated iterator, and some of the newer ones, like PeekingIterator, that dont. I would prefer to not provide access at all and deprecate the existing getIterator() methods for future versions. There are some weird things in the API that were added a long time ago, and I did not want to change everything when doing the 4.0 release, but we can decide to gradually improve the API and deprecate things that do not make sense anymore.
        Hide
        Radford Tam added a comment -

        Ok cool.

        Question though - if this is an Iterator decorator, don't we want a getIterator() method? That method would have been inherited from AbstractUntypedIteratorDecorator if it extended AbstractIteratorDecorator like it was originally written to do.

        Show
        Radford Tam added a comment - Ok cool. Question though - if this is an Iterator decorator, don't we want a getIterator() method? That method would have been inherited from AbstractUntypedIteratorDecorator if it extended AbstractIteratorDecorator like it was originally written to do.
        Hide
        Thomas Neidhart added a comment -

        Committed patch with some additional changes in r1546220:

        • added factory methods to be consistent with other decorators
        • added corresponding methods boundedIterator in IteratorUtils
        • remove unneeded variable firstNextCalled: pos can be used for this purpose
        • changed logic of next and hasNext slightly: avoid calling iterator.hasNext twice
        • do not extends AbstractIteratorDecorator: makes code slightly more readable by avoiding calling
          super.hasNext and super.next and we need to implement all methods anyway thus no real benefit of extending the decorator.
        • javadoc updates

        Thanks for this contribution!

        If you have ideas for other useful iterators, feel free to create already another ticket.

        Show
        Thomas Neidhart added a comment - Committed patch with some additional changes in r1546220: added factory methods to be consistent with other decorators added corresponding methods boundedIterator in IteratorUtils remove unneeded variable firstNextCalled: pos can be used for this purpose changed logic of next and hasNext slightly: avoid calling iterator.hasNext twice do not extends AbstractIteratorDecorator: makes code slightly more readable by avoiding calling super.hasNext and super.next and we need to implement all methods anyway thus no real benefit of extending the decorator. javadoc updates Thanks for this contribution! If you have ideas for other useful iterators, feel free to create already another ticket.
        Hide
        Radford Tam added a comment -

        Ok, I've reworked both the class and the test.

        1) Apache license header has been added.
        2) Apache commons code format has been applied.
        3) Constructor BoundedIterator(Iterator, max) has been added.
        4) The remove method has been implemented and corresponding tests added.

        Show
        Radford Tam added a comment - Ok, I've reworked both the class and the test. 1) Apache license header has been added. 2) Apache commons code format has been applied. 3) Constructor BoundedIterator(Iterator, max) has been added. 4) The remove method has been implemented and corresponding tests added.
        Hide
        Radford Tam added a comment -

        Sure thing. Let me work on it. Thanks.

        Should I just replace the attached files here when I'm done?

        Show
        Radford Tam added a comment - Sure thing. Let me work on it. Thanks. Should I just replace the attached files here when I'm done?
        Hide
        Thomas Neidhart added a comment -

        Hi Radford,

        I did review your patch and it looks pretty good.
        Just a few remarks for your future contributions:

        • you need to add an Apache license header, take a look at some other source file
        • the patch needs to be compliant to the coding style of the project
        • a constructor BoundedIterator(Iterator, max) would be handy
        • the remove method should be implemented too

        The rest looks fine. I am usually not super strict about formatting, but the more it is compliant the easier it is for the committer (= the faster it gets integrated).

        btw. we are currently in the process of releasing collections 4.0, so I can commit your patches only after this is finished.

        Show
        Thomas Neidhart added a comment - Hi Radford, I did review your patch and it looks pretty good. Just a few remarks for your future contributions: you need to add an Apache license header, take a look at some other source file the patch needs to be compliant to the coding style of the project the following formatter should work fine: http://people.apache.org/~luc/Apache-commons.xml try checking your patch with checkstyle, the configuration for collections can be found here: src/conf/checkstyle.xml a constructor BoundedIterator(Iterator, max) would be handy the remove method should be implemented too The rest looks fine. I am usually not super strict about formatting, but the more it is compliant the easier it is for the committer (= the faster it gets integrated). btw. we are currently in the process of releasing collections 4.0, so I can commit your patches only after this is finished.
        Hide
        Radford Tam added a comment -

        Here is the BoundedIterator and a JUnit test for it.
        Let me know if there's a better way for me to submit the code, and let me know what I should change or add to the class or the unit test.

        Show
        Radford Tam added a comment - Here is the BoundedIterator and a JUnit test for it. Let me know if there's a better way for me to submit the code, and let me know what I should change or add to the class or the unit test.
        Hide
        Thomas Neidhart added a comment -

        Just the files is easier.

        Show
        Thomas Neidhart added a comment - Just the files is easier.
        Hide
        Radford Tam added a comment -

        Speaking of the patch - since this is a new class (and thus, new file), would the patch be a diff of the directory in which it resides?

        Show
        Radford Tam added a comment - Speaking of the patch - since this is a new class (and thus, new file), would the patch be a diff of the directory in which it resides?
        Hide
        Thomas Neidhart added a comment -

        Sounds good. Just take a look at some existing tests for other iterators and include one in your patch.

        Looking forward to review your first contribution.

        Show
        Thomas Neidhart added a comment - Sounds good. Just take a look at some existing tests for other iterators and include one in your patch. Looking forward to review your first contribution.
        Hide
        Radford Tam added a comment -

        Hi, I've never contributed to an Apache project before, and I'm interested in doing so, perhaps starting with this issue.

        Question - The Jackrabbit project has a BoundedIterator class. Would something like that be ok for this?
        http://jackrabbit.apache.org/api/2.4/org/apache/jackrabbit/spi/commons/iterator/BoundedIterator.html

        Show
        Radford Tam added a comment - Hi, I've never contributed to an Apache project before, and I'm interested in doing so, perhaps starting with this issue. Question - The Jackrabbit project has a BoundedIterator class. Would something like that be ok for this? http://jackrabbit.apache.org/api/2.4/org/apache/jackrabbit/spi/commons/iterator/BoundedIterator.html

          People

          • Assignee:
            Unassigned
            Reporter:
            Thomas Neidhart
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development