commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [math] Add Pair factory method, toString(), Comparator
Date Wed, 16 Oct 2013 22:23:59 GMT
On Wed, 16 Oct 2013 22:43:18 +0100, Sean Owen wrote:
> No, it's the current implementation that is technically wrong. 
> Pair.java
> defines equality as equality of both keys and values. Comparator 
> orderings
> are supposed to be consistent with equals() (
> http://docs.oracle.com/javase/7/docs/api/java/util/Comparator.html). 
> 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.

In this case, it is just what we want: sort based on the keys (which 
are the
"Double"s which must be sorted). A "Pair" is used only so that the 
value part
"follows" its key during the sort. Two equal keys must compare to 0.

> 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.

Indeed it doesn't matter: in this case, pairs [1.5, 3] and [1.5, 7] can 
either
be sorted as
  [1.5, 3]
  [1.5, 7]
or
  [1.5, 7]
  [1.5, 3]
The only requirement is that they end up such that all the same keys 
are in
contiguous locations.

Consequently, the lexicographic comparator will definitely be slower 
since it
will needlessly spend time ordering the values.

>
> I prefer correctness over speed as anyone would,

Anyone should, indeed.

> and thought it was a
> decent motivation for this addition -- hey, even CM is a customer for 
> this
> class, let alone external consumers.

If we exclude the above case, which other CM code is a customer?

> I sense you don't want to commit it,

I question the correctness of the change in "sortInPlace" (with the 
above
argument), and the immediate usefulness _for_ CM (since a user 
interested
in a pair functionality should probably use Lang's implementation).
You did not expose your use-cases so I cannot judge whether a user 
would
have reasons to prefer CM's implementation.

> 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.

Neither did I; it is only testimony that there is more to maintainable 
code
than adding the code to the repository.


Regards,
Gilles

>
> On Wed, Oct 16, 2013 at 10:26 PM, Gilles 
> <gilles@harfang.homelinux.org>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).
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message