CFR - updated 8001667: Comparator combinators and extension methods

[Michael Hixson]

On Wed, Mar 6, 2013 at 1:01 PM, Henry Jen <henry.jen at oracle.com> wrote:
> On 03/06/2013 02:31 AM, Michael Hixson wrote:
>
>> 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);
>

The idea is that I wanted to use both comparators.  It's important
that "a" remains Comparator<CharSequence> because I want to sort
List<CharSequence> objects with it and use it to generate other
CharSequence-subclass comparators in addition to "b".

Also, min/max will need <S extends T> or else it will break Guava's
Ordering class.  The same thing happened a while back with
comparator.reverse() (which was then renamed to reverseOrder).  If the
only reason for the rename was to unbreak Guava, then if you use <S
extends T> you could change it back because the signatures will match.

(Maybe the Guava devs have more insight about this signature?  They
went that route for most of Ordering's methods.  Some of the same
reasoning might apply here.)

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

Regarding comparing(Function,comparator) and nulls - I'm possibly just
repeating old arguments but I'll give it another shot.

There are use cases for these all over the code base I ported to Java
8.  I'll repost the example from my first email since I think that may
answer your question about nulls:

  // Sort a list of people by:
  //  1) Last name, ignoring case
  //  2) Email address, with no email (null) last, ignoring case
  people.sort(
      comparing(Person::getLastName, CASE_INSENSITIVE_ORDER)
          .thenComparing(
              Person::getEmailAddress,
              nullsLast().thenComparing(CASE_INSENSITIVE_ORDER)));

The Comparators class itself presents four use cases for
comparing(Function,Comparator).  I don't think they're especially good
cases, but: naturalOrderKeys, naturalOrderValues, byKey, byValue could
all be done instead as comparing(Map.Entry::get{Key,Value},c).  It is
odd to me that four specialized versions of this are being offered (I
can't recall the last time I wanted to sort map entries) but the more
primitive operation behind them is not.

-Michael