ThreeTen / threeten

This project was the home of code used to develop a modern date and time library for JDK8. Development has moved to OpenJDK and a separate backport project, threetenbp.
http://threeten.github.io/
191 stars 37 forks source link

ChronoLocalDate periodUntil should return TemporalAmount #327

Closed JeremyBetts closed 10 years ago

jodastephen commented 10 years ago

Why?

JeremyBetts commented 10 years ago

TemporalAmount is the interface, which should be used instead of the class, you're forcing the return of a Period class. Rather than any implementation of TemporalAmount.

JeremyBetts commented 10 years ago

to my specific case, we're creating wrapper classes since they're declared final. We'd like to return our wrapper for Period instead of a Period directly.

JeremyBetts commented 10 years ago

To make this more general, It would be highly advisable to have all interface method signatures use the other available interfaces for both return types and argument types, unless there is absolutely no other way. using "final" classes blocks both Inheritance and Composition as options and makes the API very ridged.

RogerRiggs commented 10 years ago

There are conflicting and overlapping design goals at work. ChronoLocalDate can be considered to be part of the implementation even though it is an interface. A primary goal of the API was to provide concrete final types to the consumers of the API. Only with concrete final types can the developer be certain of immutability, no side-effects, no interposing or modifications of the semantics. The Temporal* interfaces are too abstract to use for anything but a framework glue. They are sufficient to interface between/with JSR 310 and with (some) other Date/Time packages.

jodastephen commented 10 years ago

The JSR-310 API is not an API in the style of collections.

In the collections API, the idea is that developers hold the interface, not the concrete class. As such, the interface has the complete set of methods that you want/need. Developers write List<String> list = new ArrayList<>(). ie. the LHS is the interface, not the class.

This API is modelled more closely on Integer, Double and Number. In that API, developers use the concrete classes Integer and Double. They would write code like Integer val = Integer.valueOf(6) (obviously they wouldn't really write this, but its the pattern that the API uses). As such the LHS is the concrete type, not the abstract type Number.

This API is modelled in the way it is because the principal types involved are value types, exactly like Integer and Double. In fact at future JDK version, they might be able to be optimised to real "primitive" value types, exactly like int and double. The concrete types are immutable, thread-safe and certain to use safely. Interfaces cannot make those guarantees.

This means that the interfaces, like Temporal and TemporalAmount are there primarily as "interchange" APIs. They are designed to be a better Number. Number is actually very hard to extract data from. All the methods you can call are capable of losing data, as such the only accurate way to extract data from Number is to use instanceof. Temporal and friends are designed to provide ways to extract more information than that.

ChronoLocalDate is an intermediate interface. It is intended primarily for advanced globalization use cases, where developers think that it is a good idea to hold a date in an unknown calendar system (I personally think holding such a date is a very bad idea and highly likely to result in bugs, as per the Javadoc of that class) .

ChronoLocalDate is most definitely not intended to be an abstraction of LocalDate like List is an abstraction of ArrayList. ChronoLocalDate should only be implemented when writing a new calendar system. As such, it is correct that it returns Period, because ChronoLocalDate is effectively a final class just like LocalDate, its just that Java has no such construct to handle what we want.

I am interested in your use case however. Why is there a need to wrap the API?

JeremyBetts commented 10 years ago

There are a few reasons to wrap. We’ve had an internal time package which we’d like to replace. Part of the reason is safety, we’re not willing to place direct references to the new java time objects in our code. It would leave us entirely at the mercy of a third party. What if a class has a major flaw we need to fix, we can’t wait months for whenever a java release fixes a bug.

2nd the new java time api, does not meet all our needs. Also, Absent are Spans,(a Duration/Period that has an anchor date on the timeline) Also, the Date based Period class does not have a child class that also contains a time element. We cannot Implement this class and also implement a method from the interface that returns this new object.

The API “as is” violates the Open/closed principle of Object Oriented design. It is acceptable to design an API with “final” immutable classes, however, as a tradeoff; all interfaces they implement MUST only use other interfaces/primitives in order to preserve extensibility.

http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29

There are actually very few method signatures in the interfaces that are violating this ( although I’ve only checked the classes in Java.time). Also, they all seem to have an acceptable interface alternative. My thought is that it’s a relatively minor change now, while once java 8 is released; your interfaces are set in stone for all time.

jodastephen commented 10 years ago

Thank you for explaining you intentions. I would encourage you to help fix any issues you find, so that you no longer need to wrap.

I cannot change the specific until(ChronoLocalDate) method to TemporalAmount. That method will be actively used by those teams working only with the globalized API. While I might not encourage using the globalized API, I recognise that some companies, including Oracle itself, have a strong requirement to do so. Changing the return type to TemporalAmount would make their coding experience significantly worse.

I will raise a separate issue for a possible alternative.

jodastephen commented 10 years ago

Closing as Wont Fix