Comparator/Comparators API change proposal

[Michael Hixson]

On Mon, Jun 3, 2013 at 6:13 PM, Henry Jen <henry.jen at oracle.com> wrote:
> On Jun 3, 2013, at 5:07 PM, Michael Hixson <michael.hixson at gmail.com> wrote:
>
>> 3. There are a couple of awkward-sounding parts in the docs, e.g. "Try
>> to compare null with returned comparator will throw
>> NullPointerException."  Do you want me (and/or others) to suggest
>> concrete changes here, in this thread?
>>
>
> I am open for suggestions. Thanks for reviewing.
>

Here are my suggestions.

---------------------

In Comparator:

1. "Try to compare null with returned comparator will throw
NullPointerException."

This sentence appears on Comparator.reverseOrder() and
Comparator.naturalOrder().  I'm not sure it's worth fixing the
sentence, because neither comparator strictly guarantees an NPE will
be thrown.  Maybe just remove it?

2. Comparators.comparing(Function, Comparator) has this parameter:

"cmp - the Comparator used to compare the sort key"

I suggest renaming it to "keyComparator".

3. Also on that method, the @return declaration should be fixed.

Currently:  "A comparator compares by extracted key using specified Comparator"

Should be:  "A comparator that compares by an extracted key using the
specified Comparator"

4. Similarly, Comparators.comparing(Function) and the primitive
versions' @return declarations should be fixed.

Currently:  "A comparator compares by extracted key"

Should be:  "A comparator that compares by an extracted key"

---------------------

In Comparators:

5. The descriptions of Comparators.nullsFirst(Comparator) and
nullsLast could use improving.  I don't quite understand the
motivation for this particular implementation of these comparators, so
I'll just suggest changes to improve the grammar.

Currently:  "Returns a null-friendly comparator considers null is less
than non-null. When both are null, they are considered equal. If both
are non-null, the specified Comparator is used to determine the
order."

Should be:  "Returns a null-friendly comparator that considers null to
be less than non-null. When both are null, they are considered equal.
If both are non-null, the specified Comparator is used to determine
the order."

And the @return...

Currently:  "A comparator that considers null is less than non-null."

Should be:  "A comparator that considers null to be less than non-null.

(Likewise for nullsLast.)

6. Comparators.naturalOrderKeys(), naturalOrderValues, byKey, and
byValue all could use the same fix to their docs.

Currently:  "Gets a comparator compares ..."

Should be:  "Returns a comparator that compares ..."

Their @return declarations should have the same fix.

Currently:  "A comparator compares ..."

Should be:  "A comparator that compares ..."

7. The class-level docs for Comparators could use some love.  Ideally
they would explain what the methods are doing there instead of on the
Comparator interface itself.  I didn't grok the explanation in this
thread, so in the mean time, I'll suggest a grammar fix.

Currently:  "This class consists of static utility methods for
comparators. Mostly factory method that returns a Comparator."

Should be:  "This class consists of static utility methods for
comparators. Most of the methods of this class are factory methods
that return a Comparator.

---------------------

-Michael