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
