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 <[email protected]> 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 >
