CFR - updated 8001667: Comparator combinators and extension methods

[Henry Jen]

On 03/06/2013 02:31 AM, Michael Hixson wrote:
> 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. :)
> 

I find your earlier posts on lambda-libs-spec-comments archive. I was
not on that list.

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

I think that make sense, will need to do some experiment to make sure it
won't confuse type system when chaining all together, and I am not sure
how much burden this has on inference engine. I prefer simplicity.

Theoretically, if all function take Comparator carefully allows super
type, which we have, isn't that enough? Your above example can be,

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


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

Note that Function form must return a Comparable, which implies an order
already. Combine with Comparator.reverseOrder() method, that would cover
the ground.

If the Comparable is not a fit, probably write a Comparator in lambda is
needed anyway?

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

Not sure what do you propose to be added. NULL is a controversial topic,
only application know what it means. Therefore, avoid try to interpret
null is probably a better approach. Write a Comparator if needed.

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

+1.

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

I agree.

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

My hope is this to be fixed.

Cheers,
Henry