This repository has been archived by the owner. It is now read-only.

Behaviour of compareTo() and related #132

Closed
jodastephen opened this Issue Nov 1, 2012 · 23 comments

Comments

Projects
None yet
4 participants
@jodastephen
Member

jodastephen commented Nov 1, 2012

Following the calendar system hierarchy changes, the compareTo() method definitions have changed.

A 310 goal has been that the value type classes should have equals() and compareTo() work together such that compareTo() only returns zero when equals() is true. This fits with a potential future with operator overloading. I also note that Joda-Time had a performance hotspot around the implementation of compareTo)

The equals() method definition is effectively fixed. It has to check the effective state of the date (LocalDate or FooDate). That means it checks both the date on the time-line and the chronology. Checking of the object type is optional, but it is simpler to do so, thus LocalDate is only equal to LocalDate.

The equals method implies that compareTo is an implementation of Comparable<LocalDate>, as only that could meet the requirement in the second paragraph - date, chronology and class.

Inheriting Comparable<ChronoLocalDate<?>> or Comparable<ChronoLocalDate<C>> is thus a problem.

The isAfter(), isBefore() and equalsDate() methods sidestep the problem by being defined against the date time-line only. Note this is not how they are currently implemented = a bug.

Observation 1: Semantically, the most desired comparison for ChronoLocalDate is time-line order ignoring the chronology.

Option 1: Is it necessary for ChronoLocalDate to be Comparable? Would a separate comparator be acceptable?

Option 2: Use Comparable<ChronoLocalDate<C>> and expand LocalDate.equals() to handle any arbitrary ChronoLocalDate<ISOChronology>. Note that this doesn't satisfy the most desired sort order for ChronoLocalDate.

Other options...

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 1, 2012

Contributor

java.util.Date and java.util.Calendar use the natural ordering and do not maintain "consistent with equals". This is consistent with Observation 1.

Contributor

RogerRiggs commented Nov 1, 2012

java.util.Date and java.util.Calendar use the natural ordering and do not maintain "consistent with equals". This is consistent with Observation 1.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 1, 2012

Member

I'm working hard to ensure that the key classes can have operator overloading in the future if JVM value types are introduced in a backwards compatible way. That requires the restriction in para 2.

Option 3: Use Comparable<LocalDate> on ChronoLocalDate. While initially seeming a little weird, it actually meets both requirements - a speific comparison for LocalDate and a time-line based comparison for chrono dates. BUT, it cannot be used to sort a list of ChronoLocalDate<?>.

Member

jodastephen commented Nov 1, 2012

I'm working hard to ensure that the key classes can have operator overloading in the future if JVM value types are introduced in a backwards compatible way. That requires the restriction in para 2.

Option 3: Use Comparable<LocalDate> on ChronoLocalDate. While initially seeming a little weird, it actually meets both requirements - a speific comparison for LocalDate and a time-line based comparison for chrono dates. BUT, it cannot be used to sort a list of ChronoLocalDate<?>.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 1, 2012

Contributor

Para 2 is a tighter constraint than necessary and being forced to meet it contradicts the general contract of Comparable to support the natural ordering of the type. Any future language change will need to take into account the allowable difference between equals() and Comparable == 0. The contract for Comparable is only that equals()== true then Comparable == 0 but not the other way around.

Is the argument about a specific compareTo for LocalDate based on performance? The implementation can special case the argument of a LocalDate for performance reasons. A separate LocalDate.compareTo(LocalDate) method would normally be more tightly bound by the compiler. The result would be the same regardless of which method was used.

Option 3 would effectively disable the collection classes for sorted Dates (except for LocalDate) since it would not be possible to do the necessary comparisons to do the sorting. I don't know of a declaration syntax to prevent TreeSet<ChronoDate<?>>.

Contributor

RogerRiggs commented Nov 1, 2012

Para 2 is a tighter constraint than necessary and being forced to meet it contradicts the general contract of Comparable to support the natural ordering of the type. Any future language change will need to take into account the allowable difference between equals() and Comparable == 0. The contract for Comparable is only that equals()== true then Comparable == 0 but not the other way around.

Is the argument about a specific compareTo for LocalDate based on performance? The implementation can special case the argument of a LocalDate for performance reasons. A separate LocalDate.compareTo(LocalDate) method would normally be more tightly bound by the compiler. The result would be the same regardless of which method was used.

Option 3 would effectively disable the collection classes for sorted Dates (except for LocalDate) since it would not be possible to do the necessary comparisons to do the sorting. I don't know of a declaration syntax to prevent TreeSet<ChronoDate<?>>.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 1, 2012

Member

Para 2 is not a nice to have its essential to real developers and causes all sorts of problems when broken. I've had to hack around the problem in BigDecimal on a number of occasions (bugs), and it is the cause of various odd pieces of behaviour in the collections lilbrary (which cause more bugs).

"It is strongly recommended (though not required) that natural orderings be consistent with equals. This is so because sorted sets (and sorted maps) without explicit comparators behave "strangely" when they are used with elements (or keys) whose natural ordering is inconsistent with equals. In particular, such a sorted set (or sorted map) violates the general contract for set (or map), which is defined in terms of the equals method"

As noted in Effective Java, inheritance problems apply to both equals and compareTo, and the generally accepted solution is composition, not inheritance - one reason why I argued against inheritance. Seeing as we now have a form of inheritance, we have to make it work as best we can.

Member

jodastephen commented Nov 1, 2012

Para 2 is not a nice to have its essential to real developers and causes all sorts of problems when broken. I've had to hack around the problem in BigDecimal on a number of occasions (bugs), and it is the cause of various odd pieces of behaviour in the collections lilbrary (which cause more bugs).

"It is strongly recommended (though not required) that natural orderings be consistent with equals. This is so because sorted sets (and sorted maps) without explicit comparators behave "strangely" when they are used with elements (or keys) whose natural ordering is inconsistent with equals. In particular, such a sorted set (or sorted map) violates the general contract for set (or map), which is defined in terms of the equals method"

As noted in Effective Java, inheritance problems apply to both equals and compareTo, and the generally accepted solution is composition, not inheritance - one reason why I argued against inheritance. Seeing as we now have a form of inheritance, we have to make it work as best we can.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 1, 2012

Member

Three options

(A) the Chrono level does not implement Comparable - only LocalDate/MinguoDate/CopticDate do

(B) implement Comparable<ChronoLocalDate<C>> as now

(C) implement Comparable<LocalDate>

Option (C) cause problems if MinguoDate or similar are made public in the future (as then equals/compareTo would clash). This doesn't really work.

Option (B) requires LocalDate to handle arbitrary ChronoLocalDate implementations in equals() which is a performance drain in a critical method. The same applies to compareTo(). (While your special override suggestion would work in some cases, generally comparable is used via the interface).

It also causes runtime errors, as most chrono-level developers will be using List<ChronoLocalDate<?>> not List<ChronoLocalDate<FooChronology>>. Yet Collections.sort() will happily accept the ? form while the comparable implementations will not.

Option (A) causes no new issues. Developers at the chrono-level must actively choose to use a comparator, something which makes perfect sense, as they are having to choose to throw away some information held in the objects (the chronology) while doing the comparison. The comparator can be on ChronoLocalDate, similar to:

public static final Comparator DATE_COMPARATOR = EPOCH_DAYS;

Finally, in option A, the immutable date classes like MinguoDate implement Comparable even though that will not be publicly visible in the type system. Some classes, like TreeMap work with types without requiring the visible Comparable nature - sadly Collections.sort does not.

Personally I don't think there is a choice here...

Member

jodastephen commented Nov 1, 2012

Three options

(A) the Chrono level does not implement Comparable - only LocalDate/MinguoDate/CopticDate do

(B) implement Comparable<ChronoLocalDate<C>> as now

(C) implement Comparable<LocalDate>

Option (C) cause problems if MinguoDate or similar are made public in the future (as then equals/compareTo would clash). This doesn't really work.

Option (B) requires LocalDate to handle arbitrary ChronoLocalDate implementations in equals() which is a performance drain in a critical method. The same applies to compareTo(). (While your special override suggestion would work in some cases, generally comparable is used via the interface).

It also causes runtime errors, as most chrono-level developers will be using List<ChronoLocalDate<?>> not List<ChronoLocalDate<FooChronology>>. Yet Collections.sort() will happily accept the ? form while the comparable implementations will not.

Option (A) causes no new issues. Developers at the chrono-level must actively choose to use a comparator, something which makes perfect sense, as they are having to choose to throw away some information held in the objects (the chronology) while doing the comparison. The comparator can be on ChronoLocalDate, similar to:

public static final Comparator DATE_COMPARATOR = EPOCH_DAYS;

Finally, in option A, the immutable date classes like MinguoDate implement Comparable even though that will not be publicly visible in the type system. Some classes, like TreeMap work with types without requiring the visible Comparable nature - sadly Collections.sort does not.

Personally I don't think there is a choice here...

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 1, 2012

Contributor

I don't agree that the "consistent with equals" requirement must be met by DateTime. The Comparable<Chronology<?>> construct and implementation has a clean definition and does not lead to runtime exceptions due to a constraint not enforceable at compile time. The potential for explicit comparators can be used regardless of the default behavior to be more specific about the comparison but is less usable because it forces extra coding even for the expected case.

Can you be more specific about the kinds of errors you had to hack around with BigDecimal?

This needs more time to evaluate.

Contributor

RogerRiggs commented Nov 1, 2012

I don't agree that the "consistent with equals" requirement must be met by DateTime. The Comparable<Chronology<?>> construct and implementation has a clean definition and does not lead to runtime exceptions due to a constraint not enforceable at compile time. The potential for explicit comparators can be used regardless of the default behavior to be more specific about the comparison but is less usable because it forces extra coding even for the expected case.

Can you be more specific about the kinds of errors you had to hack around with BigDecimal?

This needs more time to evaluate.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 2, 2012

Member

The problems with BigDecimal are well known and well-disliked:
http://stackoverflow.com/questions/1345619/does-scalas-bigdecimal-violate-the-equals-hashcode-contract
http://stackoverflow.com/questions/10998624/can-we-use-containsbigdecimal-zero-when-reading-a-list-in-java
http://stackoverflow.com/questions/3866514/why-bigdecimal5-50-not-equals-to-bigdecimal5-5-and-how-to-work-around-th
http://stackoverflow.com/questions/6787142/bigdecimal-equals-versus-compareto

Similar issues to this case occur with Number, which (correctly) chooses to not implement Comparable:
http://stackoverflow.com/questions/480632/why-doesnt-java-lang-number-implement-comparable
http://stackoverflow.com/questions/12561485/how-to-compare-two-numbers-in-java/12561805#12561805
http://stackoverflow.com/questions/2683202/comparing-the-values-of-two-generic-numbers

j.u.Date is not a model to follow as the subclasses are horribly broken wrt comparability.

j.u.Calendar is also not a model to follow. The compareTo method is inconsistent with equals, but the correct form of words is not used (a Javadoc bug).

The errors you see are where BigDecimal is used in collections like TreeMap and code that looks perfectly sensible doesn't work.

There is an option (D) - implement Comparable<ChronoLocalDate<?>>. This would sort by date, then by chronology name. So, two dates that are the same on the time-line with the same chronology would be equal by both equals and compareTo, but if they had different chronologies they would be different by both equals and compareTo. My view is that this behaviour would be more unexpected to users than not implementing Comparable.

Member

jodastephen commented Nov 2, 2012

The problems with BigDecimal are well known and well-disliked:
http://stackoverflow.com/questions/1345619/does-scalas-bigdecimal-violate-the-equals-hashcode-contract
http://stackoverflow.com/questions/10998624/can-we-use-containsbigdecimal-zero-when-reading-a-list-in-java
http://stackoverflow.com/questions/3866514/why-bigdecimal5-50-not-equals-to-bigdecimal5-5-and-how-to-work-around-th
http://stackoverflow.com/questions/6787142/bigdecimal-equals-versus-compareto

Similar issues to this case occur with Number, which (correctly) chooses to not implement Comparable:
http://stackoverflow.com/questions/480632/why-doesnt-java-lang-number-implement-comparable
http://stackoverflow.com/questions/12561485/how-to-compare-two-numbers-in-java/12561805#12561805
http://stackoverflow.com/questions/2683202/comparing-the-values-of-two-generic-numbers

j.u.Date is not a model to follow as the subclasses are horribly broken wrt comparability.

j.u.Calendar is also not a model to follow. The compareTo method is inconsistent with equals, but the correct form of words is not used (a Javadoc bug).

The errors you see are where BigDecimal is used in collections like TreeMap and code that looks perfectly sensible doesn't work.

There is an option (D) - implement Comparable<ChronoLocalDate<?>>. This would sort by date, then by chronology name. So, two dates that are the same on the time-line with the same chronology would be equal by both equals and compareTo, but if they had different chronologies they would be different by both equals and compareTo. My view is that this behaviour would be more unexpected to users than not implementing Comparable.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 2, 2012

Contributor

Option A does not provide methods to compare dates as equal/before/after on ChronoLocalDate. Those seem to be essential for working with dates. The need for the comparisons is independent of whether they implement Comparable.

Option D seems attractive but including the chronology in the timeline would be surprising to many and a separate method to compare only using the timeline will be needed anyway.

Contributor

RogerRiggs commented Nov 2, 2012

Option A does not provide methods to compare dates as equal/before/after on ChronoLocalDate. Those seem to be essential for working with dates. The need for the comparisons is independent of whether they implement Comparable.

Option D seems attractive but including the chronology in the timeline would be surprising to many and a separate method to compare only using the timeline will be needed anyway.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 2, 2012

Contributor

Developers will understand that comparing dates with equals and compareTo is constrained to operate only on a single type whether it is LocalDate or ChronoLocalDate and that comparing across chronologies needs specific handling. For Collections and sorting, it can be via a Comparator that uses EPOCH_DAY and for direct comparisons equalsDate or similar.

The methods for comparing dates for equals, is-before, is-after, and compare are needed to support the multi-calendar API on ChronoLocalDate. These methods are needed regardless of whether the Comparable interface is present on ChronoLocalDate and are needed and would be inherited by LocalDate.

For Collections to be sortable, the concrete types need to implement Comparable but that's not an issue. BTW, Collections.sort(List, Comparator) can be used on types that do not implement Comparable. Arrays.sort uses Comparator also. Implementing Comparable is a plus.

Multiple concrete implementations of a particular chronology are not necessary and it will simplify the implementation to specify that each concrete type does not need to interoperate with another implementation of the Chrono* type. For example, the only implementation of ChronoLocalDate is LocalDate. That will also discourage anyone from trying to create competing implementations. This allows each date type to have a concrete implementation that operates only with itself. ClassCastExceptions would be expected for equals, compareTo, isBefore, and isAfter. The generics should prevent the common improper uses at compile time and the vm should optimize the casts.

If needed, overloading of equals and compareTo in LocalDate could avoid extra type checking necessary for the most generic ChronoLocalDate and Comparable interfaces.

Option B with some refinements balances the tension between API usability and performance.

  • Constrain comparison between dates/types to be of the same calendar system; exceptions are thrown in cases not prevented by the compiler; improve the APIs to maximize the compiler's ability to trace and validate chronology types.
  • compareTo has behavior consistent with equals; isBefore and isAfter are defined to be consistent with compareTo
  • ChronoLocalDates can be directly compared for before/equals/after
  • Comparable is defined to be Chronology specific. ChronoLocalDate<C extends Chronology>
  • Collections of ChronoLocalDates (of a single type) operate as expected
  • Overloaded methods can be added if there is a performance problem with casts. Most of the uses of equals are in the avoidance of creating new instances.
Contributor

RogerRiggs commented Nov 2, 2012

Developers will understand that comparing dates with equals and compareTo is constrained to operate only on a single type whether it is LocalDate or ChronoLocalDate and that comparing across chronologies needs specific handling. For Collections and sorting, it can be via a Comparator that uses EPOCH_DAY and for direct comparisons equalsDate or similar.

The methods for comparing dates for equals, is-before, is-after, and compare are needed to support the multi-calendar API on ChronoLocalDate. These methods are needed regardless of whether the Comparable interface is present on ChronoLocalDate and are needed and would be inherited by LocalDate.

For Collections to be sortable, the concrete types need to implement Comparable but that's not an issue. BTW, Collections.sort(List, Comparator) can be used on types that do not implement Comparable. Arrays.sort uses Comparator also. Implementing Comparable is a plus.

Multiple concrete implementations of a particular chronology are not necessary and it will simplify the implementation to specify that each concrete type does not need to interoperate with another implementation of the Chrono* type. For example, the only implementation of ChronoLocalDate is LocalDate. That will also discourage anyone from trying to create competing implementations. This allows each date type to have a concrete implementation that operates only with itself. ClassCastExceptions would be expected for equals, compareTo, isBefore, and isAfter. The generics should prevent the common improper uses at compile time and the vm should optimize the casts.

If needed, overloading of equals and compareTo in LocalDate could avoid extra type checking necessary for the most generic ChronoLocalDate and Comparable interfaces.

Option B with some refinements balances the tension between API usability and performance.

  • Constrain comparison between dates/types to be of the same calendar system; exceptions are thrown in cases not prevented by the compiler; improve the APIs to maximize the compiler's ability to trace and validate chronology types.
  • compareTo has behavior consistent with equals; isBefore and isAfter are defined to be consistent with compareTo
  • ChronoLocalDates can be directly compared for before/equals/after
  • Comparable is defined to be Chronology specific. ChronoLocalDate<C extends Chronology>
  • Collections of ChronoLocalDates (of a single type) operate as expected
  • Overloaded methods can be added if there is a performance problem with casts. Most of the uses of equals are in the avoidance of creating new instances.
@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 4, 2012

Member

Option A can provide the isAfter/isBefore/equalDate methods, just not compareTo. Those methods have no requirement to be limited by "consistent with equals" behaviour.

Option D is essentially the same as what we do elsewhere, such as the Offset* classes. There, comparable is defined in a manor consistent with equals, and isAfter/isBefore are time-line based. (This option provides a stable total ordering for the objects. A list of dates will be in time-line order, its just that two dates in two different chronologies will also have an order, rather than being ==0).

Option B will fail on collections of mixed date types. I think that the globalization team should comment on this, because my expectation based on what they've said so far is that they would always use ChonoLocalDate<?> and never use a specific chronology.

I believe B will cause more bugs than D. A is more immediately inconvenient but results in fewest bugs. I'm arguing strongly for D over B.

A date comparator constant makes sense for any option where the base comparison is not time-line based. (An instant comparator should be added to Offset* classes)

Option E:
The only other choice available is to define throughout 310 that equals is time-line based rather than state-based. (Personally, it would have been better to have an equalState() method as well as equals() for all objects, but that ship sailed 15+ years ago). My view is that developers expect immutable value types to have equals comparing state.

I would be willing for there to be a broader discussion in OpenJdk/Oracle on this matter. In other words, if Oracle could change BigDecimal would it change the definition of compareTo or equals?

Member

jodastephen commented Nov 4, 2012

Option A can provide the isAfter/isBefore/equalDate methods, just not compareTo. Those methods have no requirement to be limited by "consistent with equals" behaviour.

Option D is essentially the same as what we do elsewhere, such as the Offset* classes. There, comparable is defined in a manor consistent with equals, and isAfter/isBefore are time-line based. (This option provides a stable total ordering for the objects. A list of dates will be in time-line order, its just that two dates in two different chronologies will also have an order, rather than being ==0).

Option B will fail on collections of mixed date types. I think that the globalization team should comment on this, because my expectation based on what they've said so far is that they would always use ChonoLocalDate<?> and never use a specific chronology.

I believe B will cause more bugs than D. A is more immediately inconvenient but results in fewest bugs. I'm arguing strongly for D over B.

A date comparator constant makes sense for any option where the base comparison is not time-line based. (An instant comparator should be added to Offset* classes)

Option E:
The only other choice available is to define throughout 310 that equals is time-line based rather than state-based. (Personally, it would have been better to have an equalState() method as well as equals() for all objects, but that ship sailed 15+ years ago). My view is that developers expect immutable value types to have equals comparing state.

I would be willing for there to be a broader discussion in OpenJdk/Oracle on this matter. In other words, if Oracle could change BigDecimal would it change the definition of compareTo or equals?

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 5, 2012

Contributor

ChronoLocalDate does not imply mixing ChronoXX types, Javac creates distinct bindings of for the generics that are traced.
Methods written to operate on ChronoLocalDates would declare a parameter and use it consistently.

<R extends ChronoLocalDate<R>> ChronoLocalDate<R> wednesday(ChronoLocalDate<R> date) {
    return date.with(DAY_OF_WEEK, WEDNESDAY);
}
Contributor

RogerRiggs commented Nov 5, 2012

ChronoLocalDate does not imply mixing ChronoXX types, Javac creates distinct bindings of for the generics that are traced.
Methods written to operate on ChronoLocalDates would declare a parameter and use it consistently.

<R extends ChronoLocalDate<R>> ChronoLocalDate<R> wednesday(ChronoLocalDate<R> date) {
    return date.with(DAY_OF_WEEK, WEDNESDAY);
}
@sjmisterm

This comment has been minimized.

Show comment
Hide comment
@sjmisterm

sjmisterm Nov 5, 2012

Member

Having compareTo not being consistent with equals is not an option. Option D seems the best so far. However, unless I missed something, there is nothing preventing two chronologies from having the same ID, so Chronology.identityHashCode needs to be used as the last resort in comparison if two different instances have the same name.

Member

sjmisterm commented Nov 5, 2012

Having compareTo not being consistent with equals is not an option. Option D seems the best so far. However, unless I missed something, there is nothing preventing two chronologies from having the same ID, so Chronology.identityHashCode needs to be used as the last resort in comparison if two different instances have the same name.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 5, 2012

Contributor

The use cases for homogenous date-time types far outweigh those for mixed types and should be well supported by the common APIs in the JDK. The mixed date-times cases are in the minority and their design should not disable the most common use cases.

The compiler will flag most cases of mixing generics for both B and D; with both it is possible to compile code that will fail at runtime due to type mismatches.

B supports the common case of homogenous dates better than D using the familiar APIs and types including LocalDate. A few APIs operate on Comparable, but most are either agnostic at compile time or use Comparator which may still fail at runtime.

To support the homogeneous case the concrete classes should implement Comparable such as LocalDate and JapaneseDate. As the functions for sorting are implemented, there is no difference between between B and D with respect to being able to create and use mixed collections. The compareTo methods throw class cast exceptions with mixed contents and work just fine with homogenous types.

For the homengeous types, equals and Comparable.compareTo provide the best ease of use the compiler is verifying types are consistently used.
For hetrogenous types, I might reduce the support for the mixed use case and simplify isBefore/isAfter/equalsDate to just compareDate to de-emphasise comparison of dates on the timeline regardless of their type coupled and add Comparators for use when needed with Arrays and collections.

Option D to seems to conclude that the Comparable mechanism is unusable for Threeten and all comparisons must be done with Comparators or new methods. I think it is that bad but with to the 'consistent with equals' argument it is necessary to support two parallel comparison mechanisms.

I'll ask about the case with BigDecimal.

Contributor

RogerRiggs commented Nov 5, 2012

The use cases for homogenous date-time types far outweigh those for mixed types and should be well supported by the common APIs in the JDK. The mixed date-times cases are in the minority and their design should not disable the most common use cases.

The compiler will flag most cases of mixing generics for both B and D; with both it is possible to compile code that will fail at runtime due to type mismatches.

B supports the common case of homogenous dates better than D using the familiar APIs and types including LocalDate. A few APIs operate on Comparable, but most are either agnostic at compile time or use Comparator which may still fail at runtime.

To support the homogeneous case the concrete classes should implement Comparable such as LocalDate and JapaneseDate. As the functions for sorting are implemented, there is no difference between between B and D with respect to being able to create and use mixed collections. The compareTo methods throw class cast exceptions with mixed contents and work just fine with homogenous types.

For the homengeous types, equals and Comparable.compareTo provide the best ease of use the compiler is verifying types are consistently used.
For hetrogenous types, I might reduce the support for the mixed use case and simplify isBefore/isAfter/equalsDate to just compareDate to de-emphasise comparison of dates on the timeline regardless of their type coupled and add Comparators for use when needed with Arrays and collections.

Option D to seems to conclude that the Comparable mechanism is unusable for Threeten and all comparisons must be done with Comparators or new methods. I think it is that bad but with to the 'consistent with equals' argument it is necessary to support two parallel comparison mechanisms.

I'll ask about the case with BigDecimal.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 6, 2012

Member

The use cases for homogenous date-time types far outweigh those for mixed types

Really? Oracle's I18N requirements indicated that they would always refer to dates in an abstract way rather than a specific one, so this means that collections are of CLD<?>. While that doesn't mean that mixed calendar systems would be guaranteed to be added it does mean that it would be very possible (and the signature would imply that it would be the case).

As the functions for sorting are implemented, there is no difference between between B and D with respect to being able to create and use mixed collections. The compareTo methods throw class cast exceptions with mixed contents and work just fine with homogenous types.

With option B, sorting a List<ChronoLocalDate<?>> with objects of mixed chronologies would throw an exception. With option D it would succeed.

For hetrogenous types, I might reduce the support for the mixed use case and simplify isBefore/isAfter/equalsDate to just compareDate to de-emphasise comparison of dates on the timeline regardless of their type

isAfter/isBefore exist to support if statements in business logic. They are needed on LocalDate. For ChronoLocalDate I would be happy enough to only see compareDate.

Option D to seems to conclude that the Comparable mechanism is unusable for Threeten and all comparisons must be done with Comparators or new methods.

It simply observes that just as equals doesn't play well with inheritance, nor does compareTo. However, compareTo in our case also doesn't play that well with composition.

@michael, you are right about clashing chronology IDs, although I'd be willing to just put that one in the Javadoc.

Member

jodastephen commented Nov 6, 2012

The use cases for homogenous date-time types far outweigh those for mixed types

Really? Oracle's I18N requirements indicated that they would always refer to dates in an abstract way rather than a specific one, so this means that collections are of CLD<?>. While that doesn't mean that mixed calendar systems would be guaranteed to be added it does mean that it would be very possible (and the signature would imply that it would be the case).

As the functions for sorting are implemented, there is no difference between between B and D with respect to being able to create and use mixed collections. The compareTo methods throw class cast exceptions with mixed contents and work just fine with homogenous types.

With option B, sorting a List<ChronoLocalDate<?>> with objects of mixed chronologies would throw an exception. With option D it would succeed.

For hetrogenous types, I might reduce the support for the mixed use case and simplify isBefore/isAfter/equalsDate to just compareDate to de-emphasise comparison of dates on the timeline regardless of their type

isAfter/isBefore exist to support if statements in business logic. They are needed on LocalDate. For ChronoLocalDate I would be happy enough to only see compareDate.

Option D to seems to conclude that the Comparable mechanism is unusable for Threeten and all comparisons must be done with Comparators or new methods.

It simply observes that just as equals doesn't play well with inheritance, nor does compareTo. However, compareTo in our case also doesn't play that well with composition.

@michael, you are right about clashing chronology IDs, although I'd be willing to just put that one in the Javadoc.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 6, 2012

Member

Good example of why? is common is UsabilityChrono - try removing the raw types in the last method.

Member

jodastephen commented Nov 6, 2012

Good example of why? is common is UsabilityChrono - try removing the raw types in the last method.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 6, 2012

Contributor

Easy enough, declare the Chronology parameter explicitly in the method declaration and update the references to be
ChronoLocalDate<C>.

The RAW types warnings are eliminated and it is better usage. Relying on <?> is bad practice and is usually not necessary.

private static <C extends Chronology<C>> void printMonthCal(ChronoLocalDate<C> date, PrintStream out) {

    int lengthOfMonth = date.lengthOfMonth();
    ChronoLocalDate<C> end = date.with(LocalDateTimeField.DAY_OF_MONTH, lengthOfMonth);
    end = end.plus(7 - end.get(LocalDateTimeField.DAY_OF_WEEK), LocalPeriodUnit.DAYS);
    // Back up to the beginning of the week including the 1st of the month
    ChronoLocalDate<C> start = date.with(LocalDateTimeField.DAY_OF_MONTH, 1);
    start = start.minus(start.get(LocalDateTimeField.DAY_OF_WEEK), LocalPeriodUnit.DAYS);

    out.printf("%9s Month %2d, %4d%n", date.getChronology().getId(),
            date.get(LocalDateTimeField.MONTH_OF_YEAR),
            date.get(LocalDateTimeField.YEAR));
    String[] colText = {"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"};
    printMonthRow(colText, " ", out);

    String[] cell = new String[7];
    for (; start.compareTo(end) <= 0; start = start.plus(1, LocalPeriodUnit.DAYS)) {
        int ndx = start.get(LocalDateTimeField.DAY_OF_WEEK) - 1;
        cell[ndx] = Integer.toString(start.get(LocalDateTimeField.DAY_OF_MONTH));
        if (ndx == 6) {
            printMonthRow(cell, "|", out);
        }
    }
}
Contributor

RogerRiggs commented Nov 6, 2012

Easy enough, declare the Chronology parameter explicitly in the method declaration and update the references to be
ChronoLocalDate<C>.

The RAW types warnings are eliminated and it is better usage. Relying on <?> is bad practice and is usually not necessary.

private static <C extends Chronology<C>> void printMonthCal(ChronoLocalDate<C> date, PrintStream out) {

    int lengthOfMonth = date.lengthOfMonth();
    ChronoLocalDate<C> end = date.with(LocalDateTimeField.DAY_OF_MONTH, lengthOfMonth);
    end = end.plus(7 - end.get(LocalDateTimeField.DAY_OF_WEEK), LocalPeriodUnit.DAYS);
    // Back up to the beginning of the week including the 1st of the month
    ChronoLocalDate<C> start = date.with(LocalDateTimeField.DAY_OF_MONTH, 1);
    start = start.minus(start.get(LocalDateTimeField.DAY_OF_WEEK), LocalPeriodUnit.DAYS);

    out.printf("%9s Month %2d, %4d%n", date.getChronology().getId(),
            date.get(LocalDateTimeField.MONTH_OF_YEAR),
            date.get(LocalDateTimeField.YEAR));
    String[] colText = {"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"};
    printMonthRow(colText, " ", out);

    String[] cell = new String[7];
    for (; start.compareTo(end) <= 0; start = start.plus(1, LocalPeriodUnit.DAYS)) {
        int ndx = start.get(LocalDateTimeField.DAY_OF_WEEK) - 1;
        cell[ndx] = Integer.toString(start.get(LocalDateTimeField.DAY_OF_MONTH));
        if (ndx == 6) {
            printMonthRow(cell, "|", out);
        }
    }
}
@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 6, 2012

Member

I use <?> all the time. So do most other developers. The method-based generics you used above is pretty rare outside the JDK and expert level libraries.

Try rewriting the method in the way that most developers will using ? and you'll see that start.compareTo(end) will not compile. At this point, most developers will not add the method generics, but instead add a cast and a suppress warnings

All this can be solved by choosing option D, which works the same as B for homogenous, but also works for heterogenous.

Member

jodastephen commented Nov 6, 2012

I use <?> all the time. So do most other developers. The method-based generics you used above is pretty rare outside the JDK and expert level libraries.

Try rewriting the method in the way that most developers will using ? and you'll see that start.compareTo(end) will not compile. At this point, most developers will not add the method generics, but instead add a cast and a suppress warnings

All this can be solved by choosing option D, which works the same as B for homogenous, but also works for heterogenous.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 6, 2012

Contributor

Since Oracle is promoting best practices for i18n, I would expect a more disciplined approach to using generics including proper declarations and type usages. Cleaning up the declarations improves the maintainability of the code.

Contributor

RogerRiggs commented Nov 6, 2012

Since Oracle is promoting best practices for i18n, I would expect a more disciplined approach to using generics including proper declarations and type usages. Cleaning up the declarations improves the maintainability of the code.

@RogerRiggs

This comment has been minimized.

Show comment
Hide comment
@RogerRiggs

RogerRiggs Nov 6, 2012

Contributor

Ok, Option D has the fewest drawbacks.
The two different kinds of equivalence classes are warranted for dates (those including the type/calendar and those not).

Contributor

RogerRiggs commented Nov 6, 2012

Ok, Option D has the fewest drawbacks.
The two different kinds of equivalence classes are warranted for dates (those including the type/calendar and those not).

@dchiba

This comment has been minimized.

Show comment
Hide comment
@dchiba

dchiba Nov 7, 2012

It is surprising for Option D to include chronology on the timeline, as Roger pointed out.

On the other hand, as Stephen pointed out, we anticipate mixed chrono sorting scenarios (e.g. some non-ISO enabled reservation system) so ability to sort CLD<?> would be useful. Option B falls short in this respect.

I think it is fine for equals() to return false if the chronology is different, but compareTo() should return zero if the date is the same regardless of the chronology. This is because a chronology is merely a system to identify the dates, that are essentially totally universal; the earth spins around one turn a day. All dates in one chronology have a corresponding day in the others as far as they are in the defined range of dates for the chronology.

In summary, may I ask the possibility to modify Option D to remove chronology from the timeline?

Option F: implement Comparable<ChronoLocalDate<?>>. Sort by date only. The same date on different chronologies are evaluated as neither before nor after, it's the same date. (e.g. chrono1.compareTo(chrono2) == 0 )

dchiba commented Nov 7, 2012

It is surprising for Option D to include chronology on the timeline, as Roger pointed out.

On the other hand, as Stephen pointed out, we anticipate mixed chrono sorting scenarios (e.g. some non-ISO enabled reservation system) so ability to sort CLD<?> would be useful. Option B falls short in this respect.

I think it is fine for equals() to return false if the chronology is different, but compareTo() should return zero if the date is the same regardless of the chronology. This is because a chronology is merely a system to identify the dates, that are essentially totally universal; the earth spins around one turn a day. All dates in one chronology have a corresponding day in the others as far as they are in the defined range of dates for the chronology.

In summary, may I ask the possibility to modify Option D to remove chronology from the timeline?

Option F: implement Comparable<ChronoLocalDate<?>>. Sort by date only. The same date on different chronologies are evaluated as neither before nor after, it's the same date. (e.g. chrono1.compareTo(chrono2) == 0 )

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 7, 2012

Member

Option F would make compareTo inconsistent with equals which isn't acceptable as it has lots of unexpected/negative effects.

On my blog's comments, it seems like people ultimately gravitated towards not implementing Comparable (I explained the similar case on OffsetDateTime as it was easier to grasp).

I think option D with a comparator for date-order is the best option where CLD implements Comparable. Option A (don't implement Comparable) also works for me - it just forces people to use a comparator.

Member

jodastephen commented Nov 7, 2012

Option F would make compareTo inconsistent with equals which isn't acceptable as it has lots of unexpected/negative effects.

On my blog's comments, it seems like people ultimately gravitated towards not implementing Comparable (I explained the similar case on OffsetDateTime as it was easier to grasp).

I think option D with a comparator for date-order is the best option where CLD implements Comparable. Option A (don't implement Comparable) also works for me - it just forces people to use a comparator.

@dchiba

This comment has been minimized.

Show comment
Hide comment
@dchiba

dchiba Nov 8, 2012

Thank you Stephen for the link. I also discussed this with Roger and now better understand the consistency with equals. I am totally comfortable with Option D and withdraw F.

dchiba commented Nov 8, 2012

Thank you Stephen for the link. I also discussed this with Roger and now better understand the consistency with equals. I am totally comfortable with Option D and withdraw F.

@jodastephen

This comment has been minimized.

Show comment
Hide comment
@jodastephen

jodastephen Nov 13, 2012

Member

Implemented option D. All compareTo implementations are consistent with equals, and compareTo handles the full state.

Member

jodastephen commented Nov 13, 2012

Implemented option D. All compareTo implementations are consistent with equals, and compareTo handles the full state.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.