CFR - updated 8001667: Comparator combinators and extension methods

[Michael Hixson]

Hello,

I'm not one of the people that you're looking at to review this, but I
have to give this feedback anyway.  I tried to give similar feedback
on another mailing list and got no response (maybe I chose the wrong
list).  These changes have been bothering me. :)

1. Why disable the following code even though it is (theoretically) safe?

  Comparator<CharSequence> a = comparing(CharSequence::length);
  Comparator<String> b = a.thenComparing(CASE_INSENSITIVE_ORDER);

That code would compile if the signatures of the thenComparing methods
had generics like <S extends T> instead of <T>.  The example above is
conjured up and ridiculous, but I think real code will have use for
it.  Is there any downside to narrowing the return type this way?

2. There's a thenComparing(Function), but no thenComparing(Function,
Comparator).  This seems like a big omission.  Surely people will want
secondary orderings on fields by something other than natural
(null-unfriendly) ordering.  Also, a Comparators.comparing(Function,
Comparator) method would remove the need for all the Map-centric
methods that are currently in the Comparators class.  For instance:
comparing(Map.Entry::getValue, naturalOrder()).

3. There is no support for sorting of nullable values or fields.
Sorting of nullable values directly perhaps should not be encouraged,
but sorting values by nullable fields within a chained sort is
completely reasonable.  Please add some support for this, either as
chain methods on Comparator, or as factory methods on Comparators.

4. If Comparator had min(a,b) and max(a,b) methods, the
Comparators.lesserOf(c) and greaterOf(c) methods would be unnecessary.
 The min/max methods would be generally more useful than the
BinaryOperators returned from Comparators, and c::min reads better
than Comparators.lesserOf(c).

5. Comparators.reverseOrder(c) is confusing in that it has different
behavior than Collections.reverseOrder(c).  It throws an NPE.  Also,
this new method seems to be useless because one could call
c.reverseOrder() instead.  I suggest removing the method.

6. I don't see why Comparators.compose(c1,c2) is useful given that
users can call c1.thenComparing(c2).  The latter reads better; the
word "compose" does not naturally fit with comparators and has strange
connotations for those with Math backgrounds.

7. Last I checked, even this simple example did not compile:

  Comparator<String> c = comparing(String::length);

It was confused about whether I was implying a ToDoubleFunction or a
ToLongFunction, which were both wrong.  I also ran into a lot of type
inference loop problems when chaining.

Is this simply a problem with lambda evaluation that's going to be
fixed before Java 8 is officially released?  Is there something more
complex going on here that makes statements like this impossible?

If the compilation problems aren't going to be fixed prior to the
release, or if they cannot be fixed, then none of these
Comparator/Comparators additions are useful.  You would be better off
removing them.

-Michael

On Tue, Mar 5, 2013 at 11:46 AM, Henry Jen <henry.jen at oracle.com> wrote:
> Hi,
>
> Another update to reflect functional interface renames involved in the
> API, and a bug fix for a regression found earlier.
>
> CCC had been approved. Can we get it reviewed and pushed?
>
> [1] http://cr.openjdk.java.net/~henryjen/ccc/8001667.4/webrev
>
> Cheers,
> Henry
>