commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sean Owen <>
Subject Re: [math] Add Pair factory method, toString(), Comparator
Date Wed, 16 Oct 2013 21:43:18 GMT
No, it's the current implementation that is technically wrong.
defines equality as equality of both keys and values. Comparator orderings
are supposed to be consistent with equals() ( That
is, two Pairs should be equal if and only if they compare to 0. If you only
compare keys in an ordering, you will compare to 0 for un-equal Pairs
sometimes. So defining ordering just on keys can't match this definition.

This means you could start from different orderings of the same Pairs in a
List, sort them, and not get the same result. It means that in theory some
classes like TreeSet won't work with Pairs and the current Comparator. In
practice? who knows. I bet it doesn't matter for this use case.

I prefer correctness over speed as anyone would, and thought it was a
decent motivation for this addition -- hey, even CM is a customer for this
class, let alone external consumers.

I sense you don't want to commit it, so I'd say let's just close the issue,
having committed the toString() and factory method called create(). Thanks
for that much, I didn't mean to take nearly this much time.

On Wed, Oct 16, 2013 at 10:26 PM, Gilles <>wrote:
> Hmm, I now wonder whether it is at all correct to add the comparison of
> values
> in that case (and I start to wonder why the tests still pass).

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message