Commons Collections
  1. Commons Collections
  2. COLLECTIONS-218

CollectionUtils.select() does not return passed in collection

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 4.0-alpha1, 4.0
    • Component/s: Core
    • Labels:
      None

      Description

      collect has the following methods:

      Collection collect(Collection inputCollection, final Transformer transformer)
      Collection collect(Collection inputCollection, final Transformer transformer, final Collection outputCollection)

      The first creates an ArrayList(), where the second takes an outputCollection and, for convenience, returns it.

      Select (and selectReject) currently the following methods:

      Collection select(Collection inputCollection, Predicate predicate)
      void select(Collection inputCollection, Predicate predicate, Collection outputCollection)

      I propose changing the bottom method to return the passed in outputCollection. It would simplify calling code.

        Activity

        Stephen Kestle created issue -
        Hide
        Stephen Kestle added a comment -

        Uploaded patch file for proposed changes.

        I have also made changes to the two argument methods of select(), selectReject() and collect() to be simple one-liners:

        from:
        public static Collection collect(Collection inputCollection, Transformer transformer)

        { ArrayList answer = new ArrayList(inputCollection.size()); collect(inputCollection, transformer, answer); return answer; }

        to:
        public static Collection collect(Collection inputCollection, Transformer transformer)

        { return collect(inputCollection, transformer, new ArrayList(inputCollection.size())); }

        I'll leave it up to you developers to figure out if that's a good or a bad thing...

        Show
        Stephen Kestle added a comment - Uploaded patch file for proposed changes. I have also made changes to the two argument methods of select(), selectReject() and collect() to be simple one-liners: from: public static Collection collect(Collection inputCollection, Transformer transformer) { ArrayList answer = new ArrayList(inputCollection.size()); collect(inputCollection, transformer, answer); return answer; } to: public static Collection collect(Collection inputCollection, Transformer transformer) { return collect(inputCollection, transformer, new ArrayList(inputCollection.size())); } I'll leave it up to you developers to figure out if that's a good or a bad thing...
        Stephen Kestle made changes -
        Field Original Value New Value
        Attachment CollectionUtils select return.patch [ 12337131 ]
        Stephen Colebourne made changes -
        Affects Version/s 2.1.1 [ 12311992 ]
        Component/s Collection [ 12311226 ]
        Affects Version/s Nightly Builds [ 12311784 ]
        Affects Version/s 3.1 [ 12311687 ]
        Affects Version/s 2.1 [ 12311750 ]
        Affects Version/s 1.0 [ 12311812 ]
        Affects Version/s 2.0 [ 12311825 ]
        Component/s Core [ 12311222 ]
        Affects Version/s 3.3 [ 12311987 ]
        Affects Version/s 3.0 [ 12311787 ]
        Assignee Stephen Colebourne [ scolebourne ]
        Hide
        Stephen Colebourne added a comment -

        This change is logically sensible. However, it is backwards incompatible (changing a return type is binary incompatible). As such e cannot incorporate this proposal. Sorry.

        Show
        Stephen Colebourne added a comment - This change is logically sensible. However, it is backwards incompatible (changing a return type is binary incompatible). As such e cannot incorporate this proposal. Sorry.
        Stephen Colebourne made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Won't Fix [ 2 ]
        Hide
        Stephen Kestle added a comment -

        Binary compatibility only matters if the type is Serializable right? Is there something I'm missing? If someone is Serializing a static class, then it's a bit up the wall IMO.

        It's not going to break any client code, as they won't bu using a void return type.

        Is this just a business policy?

        Show
        Stephen Kestle added a comment - Binary compatibility only matters if the type is Serializable right? Is there something I'm missing? If someone is Serializing a static class, then it's a bit up the wall IMO. It's not going to break any client code, as they won't bu using a void return type. Is this just a business policy?
        Hide
        Stephen Colebourne added a comment -

        Its not intuitive, but its not just serialized classes.

        Our requirement is that other code compiled against collections will continue to work if the new jar is dropped in (without recompilation). Its just a 'feature' of Java that this can't be met.

        See http://www.eclipse.org/eclipse/development/java-api-evolution.html

        Show
        Stephen Colebourne added a comment - Its not intuitive, but its not just serialized classes. Our requirement is that other code compiled against collections will continue to work if the new jar is dropped in (without recompilation). Its just a 'feature' of Java that this can't be met. See http://www.eclipse.org/eclipse/development/java-api-evolution.html
        Hide
        Simon Kitching added a comment -

        I was a little surprised by the comment that changing void->non-void was binary incompatible, so I checked - and this is indeed the case. Thought I'd just add this comment to confirm it for anyone else wondering. It's a little odd, as method selection during compilation depends only on the parameter types, but my simple test shows it clearly:
        Exception in thread "main" java.lang.NoSuchMethodError: Target.process(Ljava/lang/String;)V
        that V at the end indicates the void return type. There is no longer a "Target.process" method taking String and returning void (because it was modified to return String) so..boom.

        Therefore I have to agree with Steven that unfortunately this change can't be applied.

        Show
        Simon Kitching added a comment - I was a little surprised by the comment that changing void->non-void was binary incompatible, so I checked - and this is indeed the case. Thought I'd just add this comment to confirm it for anyone else wondering. It's a little odd, as method selection during compilation depends only on the parameter types, but my simple test shows it clearly: Exception in thread "main" java.lang.NoSuchMethodError: Target.process(Ljava/lang/String;)V that V at the end indicates the void return type. There is no longer a "Target.process" method taking String and returning void (because it was modified to return String) so..boom. Therefore I have to agree with Steven that unfortunately this change can't be applied.
        Hide
        Stephen Colebourne added a comment -

        Implement for Generics

        Show
        Stephen Colebourne added a comment - Implement for Generics
        Stephen Colebourne made changes -
        Resolution Won't Fix [ 2 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Stephen Colebourne made changes -
        Fix Version/s Generics [ 12312131 ]
        Stephen Kestle made changes -
        Assignee Stephen Colebourne [ scolebourne ] Stephen Kestle [ shammah ]
        Hide
        Matt Benson added a comment -

        skestle resolved svn revision 593144

        Show
        Matt Benson added a comment - skestle resolved svn revision 593144
        Matt Benson made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Henri Yandell made changes -
        Fix Version/s 3.3 [ 12311987 ]
        Fix Version/s Generics [ 12312131 ]
        Henri Yandell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Mark Thomas made changes -
        Workflow jira [ 12376364 ] Default workflow, editable Closed status [ 12607271 ]
        Thomas Neidhart made changes -
        Fix Version/s 4.0 [ 12314511 ]
        Fix Version/s 4.0-beta-1 [ 12311987 ]
        Thomas Neidhart made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Thomas Neidhart made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Thomas Neidhart made changes -
        Fix Version/s 4.0-alpha1 [ 12324645 ]
        Thomas Neidhart made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        22h 37m 1 Stephen Colebourne 19/Jul/06 22:53
        Closed Closed Reopened Reopened
        1053d 2h 56m 2 Thomas Neidhart 28/Feb/13 19:36
        Reopened Reopened Resolved Resolved
        1049d 18h 19m 2 Thomas Neidhart 28/Feb/13 19:36
        Resolved Resolved Closed Closed
        931d 19h 22m 2 Thomas Neidhart 09/Nov/14 14:31

          People

          • Assignee:
            Stephen Kestle
            Reporter:
            Stephen Kestle
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development