CFR - updated 8001667: Comparator combinators and extension methods
henry.jen at oracle.com
Wed Mar 6 21:01:01 UTC 2013
On 03/06/2013 02:31 AM, Michael Hixson wrote:
> 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);
> 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
If the Comparable is not a fit, probably write a Comparator in lambda is
> 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).
> 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.
My hope is this to be fixed.
More information about the core-libs-dev