groovy
  1. groovy
  2. GROOVY-4865

Add a take method to Collections, Iterators, Arrays

    Details

    • Type: New Feature New Feature
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.1, 1.9-beta-1
    • Component/s: groovy-jdk
    • Labels:
      None
    • Number of attachments :
      1

      Description

      With regards to a thread in the groovy user list here:

      http://groovy.329449.n5.nabble.com/How-to-access-first-10-characters-even-if-string-only-have-5-td4456146.html

      A take( n ) method would be a really useful addition to the Groovy codebase

      It is assumed it will work similar to other languages, in that:

      def a = [ 1, 2, 3 ]
      
      assert a.take( 0 ) == []
      assert a.take( 1 ) == [ 1 ]
      assert a.take( 4 ) == [ 1, 2, 3 ]
      

      The method should work for Collection, String, Map and lazily for Iterator, Reader and InputStream

        Issue Links

          Activity

          Hide
          Dinko Srkoc added a comment -

          There should also be a drop( n )

          def a = [ 1, 2, 3 ]
          
          assert a.drop( 0 ) == [ 1, 2, 3 ]
          assert a.drop( 1 ) == [ 2, 3 ]
          assert a.drop( 4 ) == []
          
          Show
          Dinko Srkoc added a comment - There should also be a drop( n ) def a = [ 1, 2, 3 ] assert a.drop( 0 ) == [ 1, 2, 3 ] assert a.drop( 1 ) == [ 2, 3 ] assert a.drop( 4 ) == []
          Hide
          Tim Yates added a comment -

          Had a few minutes to come up with the implementation and tests for take with Arrays, Lists, and Iterators

          Requires JavaDoc, and a look over to make sure it's not horribly wrong.

          Show
          Tim Yates added a comment - Had a few minutes to come up with the implementation and tests for take with Arrays, Lists, and Iterators Requires JavaDoc, and a look over to make sure it's not horribly wrong.
          Hide
          Dinko Srkoc added a comment -

          Added drop with tests for List, Array, and Iterator by closely following the take implementation.
          Also, added a bit of Javadoc to both take and drop.
          Replaced Arrays.copyOf() with ugly System.arraycopy() because copyOf is the new addition to JDK 1.6.

          Took the liberty of moving take couple of methods down, because they were sitting between head and it's counterpart tail.

          Show
          Dinko Srkoc added a comment - Added drop with tests for List, Array, and Iterator by closely following the take implementation. Also, added a bit of Javadoc to both take and drop . Replaced Arrays.copyOf() with ugly System.arraycopy() because copyOf is the new addition to JDK 1.6. Took the liberty of moving take couple of methods down, because they were sitting between head and it's counterpart tail .
          Hide
          Dierk König added a comment -

          I very much advice to change the proposed method names.

          take and drop are used for lazy sequences as in http://groovyconsole.appspot.com/script/459001 .

          Accessing the first or last n entries could be named

          first n

          last n

          which would align with the respective JDK methods.

          Show
          Dierk König added a comment - I very much advice to change the proposed method names. take and drop are used for lazy sequences as in http://groovyconsole.appspot.com/script/459001 . Accessing the first or last n entries could be named first n last n which would align with the respective JDK methods.
          Hide
          Dierk König added a comment -

          This feature needs more discussion in the dev group
          with regard to consistency of the concept and API.

          Show
          Dierk König added a comment - This feature needs more discussion in the dev group with regard to consistency of the concept and API.
          Hide
          Tim Yates added a comment -

          Is it worth continuing the effort (following the take/drop nomenclature), and a function rename can occur when a decision is reached on the dev list?

          I'll ask this on the dev list as well, as I am not sure anyone will get updates from this comment (no watchers on this issue)

          Show
          Tim Yates added a comment - Is it worth continuing the effort (following the take / drop nomenclature), and a function rename can occur when a decision is reached on the dev list? I'll ask this on the dev list as well, as I am not sure anyone will get updates from this comment (no watchers on this issue)
          Hide
          Tim Yates added a comment -
          Show
          Tim Yates added a comment - Asked a question on the groovy-dev mailing list: http://groovy.329449.n5.nabble.com/Naming-for-take-drop-methods-GROOVY-4865-td4461123.html
          Hide
          Tim Yates added a comment -

          Deleted my patch, and now using Dinko's to progress with adding the other cases and tests

          Show
          Tim Yates added a comment - Deleted my patch, and now using Dinko's to progress with adding the other cases and tests
          Hide
          Tim Yates added a comment - - edited

          Third pass (includes patch "second_take..." from Dinko Srkoc)

          This is now part of the fourth pass attachment (see below)

          Added Dinko Srkoc as an author.

          Added take( n ) methods for CharSequence, InputStream and Reader along with initial tests.

          Added since 1.8.2 to all new methods

          Not sure whether the CharBuffer method for extracting from the Reader is overkill... Feels better than reading it a char at a time, but adds a dependancy on JDK having java.nio.* (I beleive this should be ok, as it has been there since 1.4)

          Code review would be really helpful as always

          Show
          Tim Yates added a comment - - edited Third pass (includes patch "second_take..." from Dinko Srkoc) This is now part of the fourth pass attachment (see below) Added Dinko Srkoc as an author. Added take( n ) methods for CharSequence , InputStream and Reader along with initial tests. Added since 1.8.2 to all new methods Not sure whether the CharBuffer method for extracting from the Reader is overkill... Feels better than reading it a char at a time, but adds a dependancy on JDK having java.nio.* (I beleive this should be ok, as it has been there since 1.4) Code review would be really helpful as always
          Hide
          Tim Yates added a comment -

          (cross-posted from the dev list)

          Been thinking about this, and I think I now agree with Dierk

          For CharSequence, Array, List and Map, the function names take and drop don't seem right, as the original object is not (and shouldn't be) mutated, so

          def a = 'tim'
          println a.take( 1 )
          

          will print t and the variable a will still be set to 'tim'

          in these situations, I believe the methods should be called first and last

          However, for Iterator, Reader and InputStream where the state of the object is changed by reading elements, I believe that take fits, so

          def a = new StringReader('tim')
          println a.take(1)
          

          will print t, and the variable a will now be the Reader containing the remaining String 'im'

          drop would (in these three cases) exhaust the input, so there would be nothing left in the Iterator, Reader or InputStream if it were interrogated again.

          So in summary, my suggestion is:

          CharSequence, Array, List and Map : first(n) and last(n)
          

          and for:

          Iterator, Reader and InputStream : take(n) and drop(n)
          
          Show
          Tim Yates added a comment - (cross-posted from the dev list) Been thinking about this, and I think I now agree with Dierk For CharSequence , Array , List and Map , the function names take and drop don't seem right, as the original object is not (and shouldn't be) mutated, so def a = 'tim' println a.take( 1 ) will print t and the variable a will still be set to 'tim' in these situations, I believe the methods should be called first and last However, for Iterator , Reader and InputStream where the state of the object is changed by reading elements, I believe that take fits, so def a = new StringReader('tim') println a.take(1) will print t , and the variable a will now be the Reader containing the remaining String 'im' drop would (in these three cases) exhaust the input, so there would be nothing left in the Iterator , Reader or InputStream if it were interrogated again. So in summary, my suggestion is: CharSequence, Array, List and Map : first(n) and last(n) and for: Iterator, Reader and InputStream : take(n) and drop(n)
          Hide
          Tim Yates added a comment -

          New attachment fourth_pass_drop_for_CharSequence.diff

          This includes the drop method for CharSequence, and the tests for it.

          Not sure what the best way to implement drop for Reader or InputStream is...

          Show
          Tim Yates added a comment - New attachment fourth_pass_drop_for_CharSequence.diff This includes the drop method for CharSequence , and the tests for it. Not sure what the best way to implement drop for Reader or InputStream is...
          Hide
          Andrew Taylor added a comment -

          Agree with Dierk that take and drop should imply lazy. I don't think laziness is particularly useful for the common case of truncating a string, but here's a link to some code for treating Iterators/Iterables as lazy sequences, include take and drop methods: https://github.com/ataylor284/iteratorutils . I'd go with the names Dierk suggests, and save take/drop for something like that.

          Show
          Andrew Taylor added a comment - Agree with Dierk that take and drop should imply lazy. I don't think laziness is particularly useful for the common case of truncating a string, but here's a link to some code for treating Iterators/Iterables as lazy sequences, include take and drop methods: https://github.com/ataylor284/iteratorutils . I'd go with the names Dierk suggests, and save take/drop for something like that.
          Hide
          Dinko Srkoc added a comment -

          Removed my patch since Tim's fourth is the base for further progress.

          Show
          Dinko Srkoc added a comment - Removed my patch since Tim's fourth is the base for further progress.
          Hide
          Tim Yates added a comment - - edited

          New patch fifth_iteration.diff

          Added drop and take for Map. Tried explaining in the javadocs that usage with a non-ordered Map could cause fun

          Added some more documentation to some of the methods...

          Think all that's left is drop for Reader and InputStream

          Show
          Tim Yates added a comment - - edited New patch fifth_iteration.diff Added drop and take for Map . Tried explaining in the javadocs that usage with a non-ordered Map could cause fun Added some more documentation to some of the methods... Think all that's left is drop for Reader and InputStream
          Hide
          Dinko Srkoc added a comment -

          Added sixth iteration patch with drop implemented for Reader and InputStream, with tests.

          Other changes:

          • fixed bug with negative parameter value in drop for list and array
          • replaced Arrays.copyOf with System.arraycopy in take for InputStream since copyOf is added in JDK 1.6
          Show
          Dinko Srkoc added a comment - Added sixth iteration patch with drop implemented for Reader and InputStream , with tests. Other changes: fixed bug with negative parameter value in drop for list and array replaced Arrays.copyOf with System.arraycopy in take for InputStream since copyOf is added in JDK 1.6
          Hide
          Tim Yates added a comment - - edited

          Do you think Iterator.drop and Iterator.take should return Iterator<T>?

          If so, do you feel Reader.drop/take and InputStream.drop/take should respect the same contract and return Readers and InputStreams?

          PS: Nice catch with the Arrays.copyOf I completely missed that...

          Show
          Tim Yates added a comment - - edited Do you think Iterator.drop and Iterator.take should return Iterator<T> ? If so, do you feel Reader.drop/take and InputStream.drop/take should respect the same contract and return Readers and InputStreams? PS: Nice catch with the Arrays.copyOf I completely missed that...
          Hide
          Tim Yates added a comment - - edited

          Added sixth_iteration_returning_iterators.diff

          Basically the same as Dinko's sixth_iteration.diff but Iterator.take and Iterator.drop return Iterator<T> rather than List<T>

          Unit tests have been updated accordingly.

          Show
          Tim Yates added a comment - - edited Added sixth_iteration_returning_iterators.diff Basically the same as Dinko's sixth_iteration.diff but Iterator.take and Iterator.drop return Iterator<T> rather than List<T> Unit tests have been updated accordingly.
          Hide
          Dinko Srkoc added a comment -

          Yes, I believe Iterator<T> should be returned. As a general rule, Foo.take/drop should return Foo. That would probably go for List.take/drop too, e.g. LinkedList.take/drop should return LinkedList.

          I think that it would be slightly surprising if Reader/InputStream didn't return Readers and InputStreams. I must say though that I'm slightly uncomfortable with Reader/InputStream since, as Peter Mondlock said(1), they aren't really sequences or collections.

          PS: thanks

          (1): http://groovy.329449.n5.nabble.com/Naming-for-take-drop-methods-GROOVY-4865-tp4461123p4469521.html

          Show
          Dinko Srkoc added a comment - Yes, I believe Iterator<T> should be returned. As a general rule, Foo.take/drop should return Foo . That would probably go for List.take/drop too, e.g. LinkedList.take/drop should return LinkedList . I think that it would be slightly surprising if Reader/InputStream didn't return Readers and InputStreams. I must say though that I'm slightly uncomfortable with Reader/InputStream since, as Peter Mondlock said(1), they aren't really sequences or collections. PS: thanks (1): http://groovy.329449.n5.nabble.com/Naming-for-take-drop-methods-GROOVY-4865-tp4461123p4469521.html
          Hide
          Tim Yates added a comment -

          Cool, so here is seventh_iteration.diff which has:

          • Iterator.take and Iterator.drop returning Iterator<T>
          • List.take and List.drop now call createSimilarList instead of new ArrayList<T> so if LinkedList.take is called, the returned List should be a LinkedList

          I have absolutely no worries about removing the code for Reader and InputStream, as these seem to be a sticking point for this feature (and one of the causes of the argument over function names)

          Show
          Tim Yates added a comment - Cool, so here is seventh_iteration.diff which has: Iterator.take and Iterator.drop returning Iterator<T> List.take and List.drop now call createSimilarList instead of new ArrayList<T> so if LinkedList.take is called, the returned List should be a LinkedList I have absolutely no worries about removing the code for Reader and InputStream , as these seem to be a sticking point for this feature (and one of the causes of the argument over function names)
          Hide
          Dinko Srkoc added a comment -

          Removed sixth_iteration.diff.

          I'm OK with removing Reader and InputStream code too, in the interest of more consistent take/drop.

          Show
          Dinko Srkoc added a comment - Removed sixth_iteration.diff . I'm OK with removing Reader and InputStream code too, in the interest of more consistent take/drop .
          Hide
          Tim Yates added a comment -

          Added eighth_iteration.diff (and removed seventh_iteration.diff

          In this version, I have:

          • Removed drop and take for Reader and InputStream
          • Added comment about Hashtable.drop to the Map.drop method javadoc (unpredicatble results)
          • Changed unit tests to test multiple types of List, Map and CharSequence
          • Added a test for Symmetry (where the JDK allows it), so it ensures that Iterator.drop returns an Iterator, LinkedList.drop returns a LinkedList, etc
          • Added groovyTestCase blocks to each method in the javadoc. Examples in the docs are worth 1000 words

          Hope it's all ok... I'll post about this update to the dev list, to see if it is more acceptable to the group

          Show
          Tim Yates added a comment - Added eighth_iteration.diff (and removed seventh_iteration.diff In this version, I have: Removed drop and take for Reader and InputStream Added comment about Hashtable.drop to the Map.drop method javadoc (unpredicatble results) Changed unit tests to test multiple types of List , Map and CharSequence Added a test for Symmetry (where the JDK allows it), so it ensures that Iterator.drop returns an Iterator , LinkedList.drop returns a LinkedList , etc Added groovyTestCase blocks to each method in the javadoc. Examples in the docs are worth 1000 words Hope it's all ok... I'll post about this update to the dev list, to see if it is more acceptable to the group
          Hide
          Tim Yates added a comment -

          Removed import java.nio.CharBuffer as it isn't needed with the removal of Reader

          Show
          Tim Yates added a comment - Removed import java.nio.CharBuffer as it isn't needed with the removal of Reader
          Hide
          Tim Yates added a comment -

          Fantastic! Thanks Guillaume! Looks like my @since 1.8.2 was too pessemistic! :-D

          Show
          Tim Yates added a comment - Fantastic! Thanks Guillaume! Looks like my @since 1.8.2 was too pessemistic! :-D
          Hide
          Tim Yates added a comment -

          Tests failed on Java 5...

          The reason is that javax.swing.text.Segment did not implement CharSequence in Java 5, but does in Java 6...

          Here's a patch script to remove that class from the tests:

          Index: src/test/groovy/GroovyMethodsTest.groovy
          ===================================================================
          --- src/test/groovy/GroovyMethodsTest.groovy	(revision 22354)
          +++ src/test/groovy/GroovyMethodsTest.groovy	(working copy)
          @@ -947,7 +947,6 @@
                   def data = [ 'groovy',      // String
                                "${'groovy'}", // GString
                                java.nio.CharBuffer.wrap( 'groovy' ),
          -                     new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ),
                                new StringBuffer( 'groovy' ),
                                new StringBuilder( 'groovy' ) ]
                   data.each {
          @@ -1015,7 +1014,6 @@
                   def data = [ 'groovy',      // String
                                "${'groovy'}", // GString
                                java.nio.CharBuffer.wrap( 'groovy' ),
          -                     new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ),
                                new StringBuffer( 'groovy' ),
                                new StringBuilder( 'groovy' ) ]
                   data.each {
          @@ -1048,7 +1046,6 @@
                     // CharSequences
                     (java.lang.String)        : new String( 'groovy' ),
                     (java.nio.CharBuffer)     : java.nio.CharBuffer.wrap( 'groovy' ),
          -          (javax.swing.text.Segment): new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ),
                   ]
                   data.each { Class clazz, object ->
                       assert clazz.isInstance( object.take( 5 ) )
          

          Sorry about that

          Show
          Tim Yates added a comment - Tests failed on Java 5... The reason is that javax.swing.text.Segment did not implement CharSequence in Java 5, but does in Java 6... Here's a patch script to remove that class from the tests: Index: src/test/groovy/GroovyMethodsTest.groovy =================================================================== --- src/test/groovy/GroovyMethodsTest.groovy (revision 22354) +++ src/test/groovy/GroovyMethodsTest.groovy (working copy) @@ -947,7 +947,6 @@ def data = [ 'groovy', // String "${'groovy'}", // GString java.nio.CharBuffer.wrap( 'groovy' ), - new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ), new StringBuffer( 'groovy' ), new StringBuilder( 'groovy' ) ] data.each { @@ -1015,7 +1014,6 @@ def data = [ 'groovy', // String "${'groovy'}", // GString java.nio.CharBuffer.wrap( 'groovy' ), - new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ), new StringBuffer( 'groovy' ), new StringBuilder( 'groovy' ) ] data.each { @@ -1048,7 +1046,6 @@ // CharSequences (java.lang.String) : new String( 'groovy' ), (java.nio.CharBuffer) : java.nio.CharBuffer.wrap( 'groovy' ), - (javax.swing.text.Segment): new javax.swing.text.Segment( 'groovy' as char[], 0, 6 ), ] data.each { Class clazz, object -> assert clazz.isInstance( object.take( 5 ) ) Sorry about that
          Hide
          Dinko Srkoc added a comment -

          Thanks from me too, Guillaume!

          Show
          Dinko Srkoc added a comment - Thanks from me too, Guillaume!
          Hide
          Rodrigo Rosenfeld Rosas added a comment -

          Shouldn't "take" also be documented in Groovy String class? Currently it isn't:

          http://groovy.codehaus.org/groovy-jdk/java/lang/String.html

          Show
          Rodrigo Rosenfeld Rosas added a comment - Shouldn't "take" also be documented in Groovy String class? Currently it isn't: http://groovy.codehaus.org/groovy-jdk/java/lang/String.html
          Hide
          Tim Yates added a comment -

          Ahhh, because take operates on the class CharSequence (which String implements), the documentation is there instead: http://groovy.codehaus.org/groovy-jdk/java/lang/CharSequence.html

          Show
          Tim Yates added a comment - Ahhh, because take operates on the class CharSequence (which String implements), the documentation is there instead: http://groovy.codehaus.org/groovy-jdk/java/lang/CharSequence.html

            People

            • Assignee:
              Guillaume Laforge
              Reporter:
              Tim Yates
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: