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
<gil...@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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
