RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]

Sun Jun 18 20:29:17 UTC 2023

On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> In java.time packages, clarify timeline order javadoc to mention "before" and "after" in the value of the `compareTo` method return values. 
>> Add javadoc @see tags to isBefore and isAfter methods
>> 
>> Replace use of "negative" and positive with "less than zero" and "greater than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates equality.
>
> Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Use {@code xxx} to highlight the comparison against the arg.
>    Update copyrights.
>  - Merge branch 'master' into 8310033-time-compareto
>  - Clarify for Duration, AbstractChronology, and Chronology
>  - Correct javadoc of compareInstant
>  - 8310033: Improve Instant.compareTo javadoc to mention before and after
>    Refine timeline order to mention before and after
>    Add javadoc @see tags to isBefore and isAfter methods

As things stand, this PR makes things worse not better I'm afraid.

src/java.base/share/classes/java/time/Duration.java line 1422:

> 1420:      *
> 1421:      * @param otherDuration the other duration to compare to, not null
> 1422:      * @return the comparator value is less than zero if the {@code otherDuration} is before,

There is no concept of before and after when talking about a `Duration`

src/java.base/share/classes/java/time/Instant.java line 1276:

> 1274:      * @param otherInstant  the other instant to compare to, not null
> 1275:      * @return the comparator value is less than zero if the {@code otherInstant} is before,
> 1276:      *          zero if they are equal, greater than zero if the {@code otherInstant} is after

Suggestion:

     * @return the comparator value - less than zero if {@code otherInstant} is before this instant,
     *          zero if they are equal, greater than zero if {@code otherInstant} is after this instant

Saying `the {@code otherInstant}` doesn't read correctly in text.

`the comparator value` needs punctuation after it in order to make sense

src/java.base/share/classes/java/time/MonthDay.java line 678:

> 676:      *
> 677:      * @param other  the other month-day to compare to, not null
> 678:      * @return the comparator value is less than zero if the {@code other} is before,

Using before/after here could be confusing, as January could be considered to be before or after July (since the year is not defined).

src/java.base/share/classes/java/time/OffsetDateTime.java line 1805:

> 1803:      *
> 1804:      * @param other  the other date-time to compare to, not null
> 1805:      * @return the comparator value is less than zero if the {@code other} is before,

As per `OffsetTime` this definition is wrong. The comparison order is not before/after.

src/java.base/share/classes/java/time/OffsetTime.java line 1284:

> 1282:      *
> 1283:      * @param other  the other time to compare to, not null
> 1284:      * @return the comparator value is less than zero if the {@code other} is before,

Using before/after here is very wrong. As described in the text above, two instances can be at the same point in time, yet still have a defined sort order.

src/java.base/share/classes/java/time/ZoneOffset.java line 717:

> 715:      *
> 716:      * @param other  the other date to compare to, not null
> 717:      * @return the comparator value is less than zero if the {@code other} is before,

It is tricky to describe an offset as before or after. If you are going to change this you will need a much better description

src/java.base/share/classes/java/time/chrono/AbstractChronology.java line 659:

> 657:      *
> 658:      * @param other  the other chronology to compare to, not null
> 659:      * @return the comparator value is less than zero if the {@code other} ID string is before,

A `Chronology` is compared by ID, not before/after

src/java.base/share/classes/java/time/chrono/ChronoZonedDateTime.java line 572:

> 570:      *
> 571:      * @param other  the other date-time to compare to, not null
> 572:      * @return the comparator value is less than zero if the {@code other} is before,

The comparison is not before/after, as the ID is taken into account

src/java.base/share/classes/java/time/chrono/Chronology.java line 810:

> 808:      *
> 809:      * @param other  the other chronology to compare to, not null
> 810:      * @return the comparator value is less than zero if the {@code other} ID string is before,

Not before/after

src/java.base/share/classes/java/time/zone/ZoneOffsetTransition.java line 403:

> 401:      *
> 402:      * @param transition  the transition to compare to, not null
> 403:      * @return the comparator value is less than zero if the {@code transition} is before,

Not before/after

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

Changes requested by scolebourne (Author).

PR Review: https://git.openjdk.org/jdk/pull/14479#pullrequestreview-1485184785
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371176
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371665
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372314
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372199
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372018
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371857
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371782
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372453
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372462
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372480