Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: lang.*
    • Labels:
      None

      Description

      [lang] is the perfect place to provide a basic typed Pair class. I have written such a class for my employer (who hasn't?) but can/will rewrite blind to avoid IP issues. I think it's also nice to go ahead and extend this to MatchedPair<T> extends Pair<T, T> as well.

      1. Pair.java
        3 kB
        Matt Benson
      2. PairTest.java
        3 kB
        Matt Benson
      3. Pair.java
        3 kB
        Henri Yandell
      4. PairTest.java
        3 kB
        Henri Yandell

        Activity

        Hide
        Henri Yandell added a comment -
        Show
        Henri Yandell added a comment - I'll commit http://www.osjava.org/genjava/multiproject/gj-core/xref/com/generationjava/util/Pair.html and you can tweak/rewrite from there
        Hide
        Matt Benson added a comment -

        How much of the LISPy stuff do you want to keep? Or are you just popping it in for IP's sake?

        Show
        Matt Benson added a comment - How much of the LISPy stuff do you want to keep? Or are you just popping it in for IP's sake?
        Hide
        Henri Yandell added a comment -

        One question is whether the weighting concept is desirable or should be removed. i.e. that less of a Pair and more of a weighted Split. I suspect it could be deleted.

        Show
        Henri Yandell added a comment - One question is whether the weighting concept is desirable or should be removed. i.e. that less of a Pair and more of a weighted Split. I suspect it could be deleted.
        Hide
        Matt Benson added a comment -

        Yeah, I've never yet taken the time to find out what all the fuss was about wrt LISP, though I plan to one day. I was basically ready to attach my proposed code as patches for discussion, having rewritten the basics quite quickly...

        Show
        Matt Benson added a comment - Yeah, I've never yet taken the time to find out what all the fuss was about wrt LISP, though I plan to one day. I was basically ready to attach my proposed code as patches for discussion, having rewritten the basics quite quickly...
        Hide
        Matt Benson added a comment -

        proposed Pair and MatchedPair code

        Show
        Matt Benson added a comment - proposed Pair and MatchedPair code
        Hide
        Henri Yandell added a comment - - edited

        It was for IP sake - I wrote it as an exercise instead of for any use. Happy to use yours instead.

        Comments on your patches:

        • Javadoc for pairOf/matchedPairOf needs filling in.
        • @since should be '3.0' and not 'Lang 3.0'
        • The static import in the tests makes me cringe, but I know you like using them. First time I looked at the code (as I looked at tests first) I went looking in the file to see where the utility pairOf method was, while wondering why you'd done such a thing.
        • I'm not sure about the Iterable concept for MatchedPair. It could be in the parent as well as an Object iteration.
        • I do like the notion of Iterable if it becomes a tree walker - i.e. it iterates down Pairs of Pairs. It could be: depthFirstIterate() & breadthFirstIterate().
        • Possibly some kind of construction parameter that sets the default to be depthFirst or breadthFirst. Or we could just declare one to be the default.
        Show
        Henri Yandell added a comment - - edited It was for IP sake - I wrote it as an exercise instead of for any use. Happy to use yours instead. Comments on your patches: Javadoc for pairOf/matchedPairOf needs filling in. @since should be '3.0' and not 'Lang 3.0' The static import in the tests makes me cringe, but I know you like using them. First time I looked at the code (as I looked at tests first) I went looking in the file to see where the utility pairOf method was, while wondering why you'd done such a thing. I'm not sure about the Iterable concept for MatchedPair. It could be in the parent as well as an Object iteration. I do like the notion of Iterable if it becomes a tree walker - i.e. it iterates down Pairs of Pairs. It could be: depthFirstIterate() & breadthFirstIterate(). Possibly some kind of construction parameter that sets the default to be depthFirst or breadthFirst. Or we could just declare one to be the default.
        Hide
        Matt Benson added a comment - - edited

        I'll respond to your first three comments in order:

        • RE Javadoc of static methods: What are you looking for, beyond completing the @param tags such that checkstyle is happy?
        • RE @since: Okay.
        • RE static imports: don't know what to say; 'tis a matter of opinion. If as a community we turn out to fall in favor of not using static imports I will abide.

        WRT MatchedPair<T> as Iterable<T>, to be honest MatchedPair exists, in my mind, mostly because only when the L and R types of Pair are the same can we provide a typed Iterator. As for the tree walker concept, that never occurred to me though I suppose I can see where you're coming from now that you mention it. Perhaps your impression of a Pair as a LISP concept and my lack of similar background account for the difference in perspective. However, as you mentioned this would only really work if we sacrifice type-safety by making Pair Iterable<Object>, and in that case it could be argued that the whole notion of iterating over nested Iterables in depth-first or breadth-first fashion could be extracted to a completely separate class, and be a more versatile approach into the bargain, no?

        In this case I'd be satisfied to drop MatchedPair as a class, and add to Pair:

            public static <T> Iterator<T> pairIterator(final Pair<? extends T, ? extends T> pair) {
                return new Iterator<T>() {
                    private int index = -1;
        
                    public boolean hasNext() {
                        return index < 1;
                    }
        
                    public T next() {
                        if (index >= 1) {
                            throw new NoSuchElementException();
                        }
                        return ++index == 0 ? pair.left : pair.right;
                    }
        
                    public void remove() {
                        throw new UnsupportedOperationException();
                    }
                };
            }
        
        Show
        Matt Benson added a comment - - edited I'll respond to your first three comments in order: RE Javadoc of static methods: What are you looking for, beyond completing the @param tags such that checkstyle is happy? RE @since: Okay. RE static imports: don't know what to say; 'tis a matter of opinion. If as a community we turn out to fall in favor of not using static imports I will abide. WRT MatchedPair<T> as Iterable<T>, to be honest MatchedPair exists, in my mind, mostly because only when the L and R types of Pair are the same can we provide a typed Iterator. As for the tree walker concept, that never occurred to me though I suppose I can see where you're coming from now that you mention it. Perhaps your impression of a Pair as a LISP concept and my lack of similar background account for the difference in perspective. However, as you mentioned this would only really work if we sacrifice type-safety by making Pair Iterable<Object>, and in that case it could be argued that the whole notion of iterating over nested Iterables in depth-first or breadth-first fashion could be extracted to a completely separate class, and be a more versatile approach into the bargain, no? In this case I'd be satisfied to drop MatchedPair as a class, and add to Pair: public static <T> Iterator<T> pairIterator( final Pair<? extends T, ? extends T> pair) { return new Iterator<T>() { private int index = -1; public boolean hasNext() { return index < 1; } public T next() { if (index >= 1) { throw new NoSuchElementException(); } return ++index == 0 ? pair.left : pair.right; } public void remove() { throw new UnsupportedOperationException(); } }; }
        Hide
        Sebb added a comment -

        Why do you need readObject()? Surely the hashCode could just be serialised along with the objects?

        If readObject() really is needed, then hashCode needs to be made volatile.
        Otherwise make it final.

        I think it would be helpful to expand the class Javadoc to include some use cases for the class.

        Show
        Sebb added a comment - Why do you need readObject()? Surely the hashCode could just be serialised along with the objects? If readObject() really is needed, then hashCode needs to be made volatile. Otherwise make it final. I think it would be helpful to expand the class Javadoc to include some use cases for the class.
        Hide
        Matt Benson added a comment - - edited

        My concern regarding the hashCode, Seb, is that a different calculation may be gotten on different running VMs. I am certainly open to making the field volatile if necessary, but TBH I don't see why it would be, unless it's possible for hashCode() be called (a) before the constructor has executed, or (b) before deserialization has completed. Educate me here.

        Show
        Matt Benson added a comment - - edited My concern regarding the hashCode, Seb, is that a different calculation may be gotten on different running VMs. I am certainly open to making the field volatile if necessary, but TBH I don't see why it would be, unless it's possible for hashCode() be called (a) before the constructor has executed, or (b) before deserialization has completed. Educate me here.
        Hide
        Sebb added a comment -

        The reason is visibility of the variable.
        There's no guarantee that a non-final variable set in one thread will be seen by other threads in the absence of some form of synch.

        It's unlikely that the problem will occur, but it's possible, and will be extremely hard to track down if it does.

        Show
        Sebb added a comment - The reason is visibility of the variable. There's no guarantee that a non-final variable set in one thread will be seen by other threads in the absence of some form of synch. It's unlikely that the problem will occur, but it's possible, and will be extremely hard to track down if it does.
        Hide
        Henri Yandell added a comment -

        What's the value in being able to iterate (without looking like an Iterable) an item with 2 things in?

        Show
        Henri Yandell added a comment - What's the value in being able to iterate (without looking like an Iterable) an item with 2 things in?
        Hide
        Matt Benson added a comment -

        Yeah, I was originally thinking someone could choose to create the Iterable and delegate the Iterator creation to that method; instead now I am thinking of making the method return an Iterable with an option to return Iterables for nested Pairs. This would probably work best for our hypothetical Depth/BreadthFirst iterations...

        Show
        Matt Benson added a comment - Yeah, I was originally thinking someone could choose to create the Iterable and delegate the Iterator creation to that method; instead now I am thinking of making the method return an Iterable with an option to return Iterables for nested Pairs. This would probably work best for our hypothetical Depth/BreadthFirst iterations...
        Hide
        Stephen Colebourne added a comment -

        I wouldn't cache the hash code in this class. If the pair contains a mutable object, then the hash code of the mutable could change during the lifetime of the Pair, causing the Pair hash code to break. Just calculate the hash code on demand.

        I'd also avoid MatchedPair if possible. I'd like to see Pair be final and immutable (dependent on contained data).

        I don't especially see the value of the iterator per se, but having a way to convert a Pair to a list might be useful. That method could have an option to expand any pairs it finds (and this could be built on an iterator internally, rather than up front extraction...)

        In general though, a [lang] Pair needs to be simple and non-religious

        Show
        Stephen Colebourne added a comment - I wouldn't cache the hash code in this class. If the pair contains a mutable object, then the hash code of the mutable could change during the lifetime of the Pair, causing the Pair hash code to break. Just calculate the hash code on demand. I'd also avoid MatchedPair if possible. I'd like to see Pair be final and immutable (dependent on contained data). I don't especially see the value of the iterator per se, but having a way to convert a Pair to a list might be useful. That method could have an option to expand any pairs it finds (and this could be built on an iterator internally, rather than up front extraction...) In general though, a [lang] Pair needs to be simple and non-religious
        Hide
        Matt Benson added a comment -

        @Stephen: do you have a reason to say "List" specifically, or would Iterable satisfy you? If so, it sounds like we're zeroing in on agreement.

        Show
        Matt Benson added a comment - @Stephen: do you have a reason to say "List" specifically, or would Iterable satisfy you? If so, it sounds like we're zeroing in on agreement.
        Hide
        Matt Benson added a comment -

        Actually I'm now having second thoughts about how much call there is for iterating Pairs , and if there is there's a fairly quick way to do it already:

        Pair<Integer, Double> pair = pairOf(10, 100d);
        List<? extends Number> l = Arrays.asList(pair.left, pair.right);
        

        So in this case, we could drop the iteration issue, calculate hashCode always, and be done?

        Show
        Matt Benson added a comment - Actually I'm now having second thoughts about how much call there is for iterating Pairs , and if there is there's a fairly quick way to do it already: Pair< Integer , Double > pair = pairOf(10, 100d); List<? extends Number > l = Arrays.asList(pair.left, pair.right); So in this case, we could drop the iteration issue, calculate hashCode always, and be done?
        Hide
        Stephen Colebourne added a comment -

        I have no specific requirement for a List. However I would advise against an Iterable, simply because its not that widely accepted in other APIs.

        The ability to merge pairs into a single list will be a requested feature I suspect.

        As I said, I'm strongly against caching the hash code - premature optimisation and unsafe with mutable classes.

        Show
        Stephen Colebourne added a comment - I have no specific requirement for a List. However I would advise against an Iterable, simply because its not that widely accepted in other APIs. The ability to merge pairs into a single list will be a requested feature I suspect. As I said, I'm strongly against caching the hash code - premature optimisation and unsafe with mutable classes.
        Hide
        Paul Benedict added a comment -

        I would advise against the L and R mnemonic. It should just be T1 and T2 or U and V, etc. We shouldn't imply its usage.

        Show
        Paul Benedict added a comment - I would advise against the L and R mnemonic. It should just be T1 and T2 or U and V, etc. We shouldn't imply its usage.
        Hide
        Matt Benson added a comment -

        I'm not that attached to L/R-left/right. That said, I like left/right because they are short, sweet and monosyllabic. They just feel good to me. Happy to bikeshed about any other possibilities that roll off the keyboard in a similar way.

        Show
        Matt Benson added a comment - I'm not that attached to L/R-left/right. That said, I like left/right because they are short, sweet and monosyllabic. They just feel good to me. Happy to bikeshed about any other possibilities that roll off the keyboard in a similar way.
        Hide
        Henri Yandell added a comment -

        Left/Right does imply tree/graph like structures are going to be built. Agreed on the niceness of the name though.

        I think we do imply order in the Pair, especially with all the iteration conversation - how about First, Second?

        Show
        Henri Yandell added a comment - Left/Right does imply tree/graph like structures are going to be built. Agreed on the niceness of the name though. I think we do imply order in the Pair, especially with all the iteration conversation - how about First, Second?
        Hide
        Matt Benson added a comment - - edited

        Hmm... I could live with those, but I'm not crazy about 'em...

        Other possibilities, some sillier than others:

        a/b?
        x/y?
        thing1/thing2?
        fric/frac?
        dee/dum?
        one/other?
        him/her?

        this and that would be nice if not for that little reserved word problem...

        Show
        Matt Benson added a comment - - edited Hmm... I could live with those, but I'm not crazy about 'em... Other possibilities, some sillier than others: a/b? x/y? thing1/thing2? fric/frac? dee/dum? one/other? him/her? this and that would be nice if not for that little reserved word problem...
        Hide
        Paul Benedict added a comment -

        I vote for T1 and T2.

        Show
        Paul Benedict added a comment - I vote for T1 and T2.
        Hide
        Matt Benson added a comment -

        So do T1/T2 type variable names correspond to the Seussian thing1/thing2 nomenclature for you, Paul?

        Show
        Matt Benson added a comment - So do T1/T2 type variable names correspond to the Seussian thing1/thing2 nomenclature for you, Paul?
        Hide
        Paul Benedict added a comment -

        They are type parameters. They correspond to type parameter 1 and type parameter 2.

        Show
        Paul Benedict added a comment - They are type parameters. They correspond to type parameter 1 and type parameter 2.
        Hide
        Matt Benson added a comment -

        I understand that, but apparently what I am not conveying is my assumption that the type parameters would carry a mnemonic correlation with the class members that carry the constituent parts of the Pair. I would thus expect, perhaps:

        left/right->L/R
        a/b->A/B
        x/y->X/Y
        thing1/thing2->T1/T2

        ...and my more facetious suggestions are perhaps too ridiculous for this exercise.

        Show
        Matt Benson added a comment - I understand that, but apparently what I am not conveying is my assumption that the type parameters would carry a mnemonic correlation with the class members that carry the constituent parts of the Pair. I would thus expect, perhaps: left/right->L/R a/b->A/B x/y->X/Y thing1/thing2->T1/T2 ...and my more facetious suggestions are perhaps too ridiculous for this exercise.
        Hide
        Paul Benedict added a comment -

        Should we provide a Pair<X, Y> interface as well as an implementation?

        Show
        Paul Benedict added a comment - Should we provide a Pair<X, Y> interface as well as an implementation?
        Hide
        Matt Benson added a comment -

        That would kind of kill the "pair members are final so let's access them via public final members" vibe, but I suppose we could put it to a vote if necessary... :|

        Show
        Matt Benson added a comment - That would kind of kill the "pair members are final so let's access them via public final members" vibe, but I suppose we could put it to a vote if necessary... :|
        Hide
        ggregory@seagullsw.com added a comment -

        In the Smalltalk world, this was called an Association with a key and value, so in Java we could have Association<K,V>.

        If you really care about providing immutable assocs, then you can a mutable and immutable interface.

        I too have written and re-written many versions of this class through time. This is really a public version of what the JRE provides in the MapEntry class of course. I usually end up with Maps of Associations.

        Show
        ggregory@seagullsw.com added a comment - In the Smalltalk world, this was called an Association with a key and value, so in Java we could have Association<K,V>. If you really care about providing immutable assocs, then you can a mutable and immutable interface. I too have written and re-written many versions of this class through time. This is really a public version of what the JRE provides in the MapEntry class of course. I usually end up with Maps of Associations.
        Hide
        gabriele renzi added a comment -

        FWIW, the Pair class we use on my $WORK is more or less the same (bar not caching the hashcode, I just xor the existing ones which should be usually ok) but there are two differences:

        • pairOf is just of , which allows us to just write Pair.of(a,b). This also goes into the direection of behing more coherent with com.sun.tools.javac.util.Pair, although not compatile anyway
        • can we have a proper toString method?
        Show
        gabriele renzi added a comment - FWIW, the Pair class we use on my $WORK is more or less the same (bar not caching the hashcode, I just xor the existing ones which should be usually ok) but there are two differences: pairOf is just of , which allows us to just write Pair.of(a,b). This also goes into the direection of behing more coherent with com.sun.tools.javac.util.Pair, although not compatile anyway can we have a proper toString method?
        Hide
        Stephen Colebourne added a comment -

        I should note that EnumSet and JSR-310 use Classname.of - its becoming a new standard. The benefit of static imports seems to wane by comparison. So, +1 to Pair.of()

        As explained above this class shouldn't cache the hash code, as mutable objects might be stashed within.

        I should say that I find the L/R notation perfectly acceptable for this class - it doesn't overly imply anything to me.

        Show
        Stephen Colebourne added a comment - I should note that EnumSet and JSR-310 use Classname.of - its becoming a new standard. The benefit of static imports seems to wane by comparison. So, +1 to Pair.of() As explained above this class shouldn't cache the hash code, as mutable objects might be stashed within. I should say that I find the L/R notation perfectly acceptable for this class - it doesn't overly imply anything to me.
        Hide
        Matt Benson added a comment -

        Okay, I know I said I was happy to bikeshed, but as it turns out I don't really feel that happy. We now have seemingly too many divergent suggestions to achieve consensus. By using interfaces we would also seem to give up a convenient place to keep our static factory methods. I will additionally note that I will make no argument against emulating the emerging of() standard if indeed it is such (I'll take your word on this, Stephen).

        Show
        Matt Benson added a comment - Okay, I know I said I was happy to bikeshed, but as it turns out I don't really feel that happy. We now have seemingly too many divergent suggestions to achieve consensus. By using interfaces we would also seem to give up a convenient place to keep our static factory methods. I will additionally note that I will make no argument against emulating the emerging of() standard if indeed it is such (I'll take your word on this, Stephen).
        Hide
        Henri Yandell added a comment -

        I think consensus isn't lost.

        I'm +1 to L/R. I think there's majority happiness there. Same for the name Pair. The iteration code is out by your own declaration.

        Show
        Henri Yandell added a comment - I think consensus isn't lost. I'm +1 to L/R. I think there's majority happiness there. Same for the name Pair. The iteration code is out by your own declaration.
        Hide
        Henri Yandell added a comment -

        Updating the Pair.java file. toString added, hashCode no longer uses cached value, pairOf method renamed to of.

        Show
        Henri Yandell added a comment - Updating the Pair.java file. toString added, hashCode no longer uses cached value, pairOf method renamed to of.
        Hide
        Henri Yandell added a comment -

        I also made the class final.

        Show
        Henri Yandell added a comment - I also made the class final.
        Hide
        Henri Yandell added a comment -

        Noting that I applied the files in r916098.

        Needing to add unit tests around the toString method before closing the issue.

        Show
        Henri Yandell added a comment - Noting that I applied the files in r916098. Needing to add unit tests around the toString method before closing the issue.
        Hide
        Henri Yandell added a comment -

        svn ci -m "Adding toString test. LANG-588"
        Sending src/test/java/org/apache/commons/lang3/PairTest.java
        Transmitting file data .
        Committed revision 916099.

        Show
        Henri Yandell added a comment - svn ci -m "Adding toString test. LANG-588 " Sending src/test/java/org/apache/commons/lang3/PairTest.java Transmitting file data . Committed revision 916099.

          People

          • Assignee:
            Unassigned
            Reporter:
            Matt Benson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development