[10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jonathan Bluett-Duncan]

You're welcome Ivan!

I'm just proofreading the new webrev, and a few more things have occurred
to me:

1. What happens if the comparators are given the elements {"1abc", "2abc",
"10abc"} to compare? Would they sort the elements as {"1abc", "2abc",
"10abc"} or as {"1abc", "10abc", "2abc"}?
What about the array {"x1yz", "x2yz", "x10yz"}?
I wonder if these two cases should be added to the test too and given as
additional examples in the javadocs.

2. The example arrays which you kindly added to the comparators' javadoc
have no whitespace at the start but one space between the last element and
the }, so I think there either should be no space at the end or an extra
space at the start.

3. Very sorry, error on my part: on the javadoc lines which now say "then the
corresponding numerical values are compared; otherwise", I suggested to add
a "then" at the start; re-reading it though, I personally think it reads
better without, but I would understand and not press it further if you
disagree and think that the "then" is a useful addition.

Best regards,
Jonathan


On 24 Jul 2017 06:21, "Ivan Gerasimov" <ivan.gerasimov at oracle.com> wrote:

Thank you Jonathan for looking into that!

I agree with all your suggestions.

Here's the updated webrev with suggested modifications:
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/02/webrev/

With kind regards,
Ivan


On 7/23/17 3:56 AM, Jonathan Bluett-Duncan wrote:

Meant to reply to core-libs-dev as well; doing so now.

Jonathan

On 23 July 2017 at 11:50, Jonathan Bluett-Duncan <jbluettduncan at gmail.com>
wrote:

> Hi Ivan,
>
> I'm not a reviewer per se, but I thought I'd chime in.
>
> There's a few things with Comparator.java which I think could be improved:
>
>    1. `comparingNumericallyLeadingZerosAhead()` is a confusing name for
>    me, as the "ahead" has no meaning in my mind; IMO the name
>    `comparingNumericallyLeadingZerosFirst()` better implies that "0001" <
>    "001" < "01".
>    2. It wasn't clear to me from the javadoc that the comparators compare
>    strings like "abc9" and "abc10" as "abc9" < "abc10", so I think they should
>    include more examples.
>    3. There's a typo in the javadocs for both methods: "represets" -->
>    "represents".
>    4. Where the javadocs say "follows the procedure", I think it'd make
>    more grammatical sense if they said "does the following".
>    5. The lines which say "at the boundary of a subsequence, consisting
>    of decimal digits, the" would flow better if they said "at the boundary of
>    a subsequence *consisting solely of decimal digits*, then the". Note
>    the removal of the comma between "subsequence" and "consisting".
>
> Hope this helps!
>
> Jonathan
>
> On 23 July 2017 at 05:36, Ivan Gerasimov <ivan.gerasimov at oracle.com>
> wrote:
>
>> Hello!
>>
>> This is a gentle reminder.
>>
>> The proposed comparator implementation would be particularly useful when
>> one will need to compare version strings.
>> Some popular file managers also use similar comparing algorithm, as the
>> results often look more natural to the human eyes (e.g. File Explorer on
>> Windows, Files on Ubuntu).
>>
>> Now, as Java 10 is been worked on, to sort the list of Java names
>> correctly, this kind of comparator is needed:
>>
>> Look: a list { ... "Java 8", "Java 9", "Java 10" } definitely looks nicer
>> than { "Java 1", "Java 10", "Java 2", ... }  :-)
>>
>> Would you please help review the proposal?
>>
>> With kind regards,
>> Ivan
>>
>>
>>
>> On 7/19/17 1:41 AM, Ivan Gerasimov wrote:
>>
>>> Hello!
>>>
>>> It is a proposal to provide a String comparator, which will pay
>>> attention to the numbers embedded into the strings (should they present).
>>>
>>> This proposal was initially discussed back in 2014 and seemed to bring
>>> some interest from the community:
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-De
>>> cember/030343.html
>>>
>>> In the latest webrev two methods are added to the public API:
>>> j.u.Comparator.comparingNumerically() and
>>> j.u.Comparator.comparingNumericallyLeadingZerosAhead().
>>>
>>> The regression test is extended to exercise this new comparator.
>>>
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8134512
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/01/webrev/
>>>
>>> Comments, suggestions are very welcome!
>>>
>>>
>> --
>> With kind regards,
>> Ivan Gerasimov
>>
>>
>

-- 
With kind regards,
Ivan Gerasimov