updated 8001667: Comparator combinators and extension methods (original) (raw)

Henry Jen henry.jen at oracle.com
Wed Mar 6 21:01:01 UTC 2013

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 a = comparing(CharSequence::length); Comparator b = a.thenComparing(CASEINSENSITIVEORDER); That code would compile if the signatures of the thenComparing methods had generics like instead of . 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 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 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

More information about the core-libs-dev mailing list