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

Consider adding ChronoPeriod #331

Closed jodastephen closed 11 years ago

jodastephen commented 11 years ago

Currently, Period is defined to be "years, months and days" where the meaning of that only happens when the class is added to a date. This definition is pragmatic, but also pretty icky. It means that a period between two ISO dates can be added to a Hijrah date, an action which isn't meaningful.

An alternative design would be to create a ChronoPeriod interface with a subset of methods from Period, plus a getChronology() method. Period would implement ChronoPeriod and be the ISO implementation (just like ChronoLocalDate and LocalDate). A private implementation would handle all non-ISO calendar systems, just like ChronoLocalDateImpl.

With this change, all periods would be chronology specific. Runtime exceptions would prevent a Hijrah-date derived period from being added to an ISO date.

This requires adding one public interface and one private class to the API, both in the chrono package.

The change is relatively low risk. It increases runtime safety and is a relatively logical change, since all references to years/months/days should have a related calendar system. However, it does make it harder to transfer periods between two related calendar systems, such as the Japanese, Thai and ISO.

Note, issue #327 triggered this idea.

jodastephen commented 11 years ago

This is an outline of the possible new interface. It will also require some additional factory methods on Chronology;

package java.time.chrono;

import java.time.DateTimeException;
import java.time.temporal.ChronoUnit;
import java.time.temporal.Temporal;
import java.time.temporal.TemporalAmount;
import java.time.temporal.TemporalUnit;
import java.time.temporal.UnsupportedTemporalTypeException;
import java.util.List;

/**
 * A date-based amount of time, such as '3 years, 4 months and 5 days' in an
 * arbitrary chronology, intended for advanced globalization use cases.
 * <p>
 * This interface models a date-based amount of time in a calendar system.
 * While most calendar systems use years, months and days, some do not.
 * Therefore, this interface operates solely in terms of a set of supported
 * units that are defined by the {@code Chronology}.
 * The set of supported units is fixed for a given chronology.
 * The amount of a supported unit may be set to zero.
 * <p>
 * The period is modeled as a directed amount of time, meaning that individual
 * parts of the period may be negative.
 *
 * @implSpec
 * This class is immutable and thread-safe.
 *
 * @since 1.8
 */
public interface ChronoPeriod
        extends TemporalAmount {

    /**
     * Obtains a {@code ChronoPeriod} consisting of amount of time between two dates.
     * <p>
     * The start date is included, but the end date is not.
     * The period is calculated using {@link ChronoLocalDate#until(ChronoLocalDate)}.
     * As such, the calculation is chronology specific.
     * <p>
     * The chronology of the first date is used.
     * The chronology of the second date is ignored, with the date being converted
     * to the target chronology system before the calculation starts.
     * <p>
     * The result of this method can be a negative period if the end is before the start.
     * In most cases, the positive/negative sign will be the same in each of the supported fields.
     *
     * @param startDate  the start date, inclusive, not null
     * @param endDate  the end date, exclusive, not null
     * @return the period between this date and the end date, not null
     * @see ChronoLocalDate#until(ChronoLocalDate)
     */
    public static ChronoPeriod between(ChronoLocalDate startDate, ChronoLocalDate endDate) {
        return startDate.until(endDate);
    }

    //-----------------------------------------------------------------------
    /**
     * Gets the value of the requested unit.
     * <p>
     * The supported units are chronology specific.
     * They will typically be {@link ChronoUnit#YEARS YEARS},
     * {@link ChronoUnit#MONTHS MONTHS} and {@link ChronoUnit#DAYS DAYS}.
     * Requesting an unsupported unit will throw an exception.
     *
     * @param unit the {@code TemporalUnit} for which to return the value
     * @return the long value of the unit
     * @throws DateTimeException if the unit is not supported
     * @throws UnsupportedTemporalTypeException if the unit is not supported
     */
    @Override
    long get(TemporalUnit unit);

    /**
     * Gets the set of units supported by this period.
     * <p>
     * The supported units are chronology specific.
     * They will typically be {@link ChronoUnit#YEARS YEARS},
     * {@link ChronoUnit#MONTHS MONTHS} and {@link ChronoUnit#DAYS DAYS}.
     * They are returned in order from largest to smallest.
     * <p>
     * This set can be used in conjunction with {@link #get(TemporalUnit)}
     * to access the entire state of the period.
     *
     * @return a list containing the supported units, not null
     */
    @Override
    List<TemporalUnit> getUnits();

    /**
     * Gets the chronology that defines the meaning of the supported units.
     * <p>
     * The period is defined by the chronology.
     * It controls the supported units and restricts addition/subtraction
     * to {@code ChronoLocalDate} instances of the same chronology.
     *
     * @return the chronology defining the period, not null
     */
    Chronology getChronology();
    // TODO: could have null mean chronology-neutral

    //-----------------------------------------------------------------------
    /**
     * Checks if all the supported units of this period are zero.
     *
     * @return true if this period is zero-length
     */
    default boolean isZero() {
        for (TemporalUnit unit : getUnits()) {
            if (get(unit) != 0) {
                return false;
            }
        }
        return true;
    }

    /**
     * Checks if any of the supported units of this period are negative.
     *
     * @return true if any unit of this period is negative
     */
    default boolean isNegative() {
        for (TemporalUnit unit : getUnits()) {
            if (get(unit) < 0) {
                return true;
            }
        }
        return false;
    }

    //-----------------------------------------------------------------------
    /**
     * Returns a copy of this period with the specified period added.
     * <p>
     * The specified period must have the same chronology.
     * This returns a period with the matching amount individually added
     * to each supported unit.
     * <p>
     * This instance is immutable and unaffected by this method call.
     *
     * @param amountToAdd  the period to add, not null
     * @return a {@code Period} based on this period with the requested period added, not null
     * @throws ArithmeticException if numeric overflow occurs
     */
    ChronoPeriod plus(ChronoPeriod amountToAdd);

    /**
     * Returns a copy of this period with the specified period subtracted.
     * <p>
     * The specified period must have the same chronology.
     * This returns a period with the matching amount individually subtracted
     * from each supported unit.
     * <p>
     * This instance is immutable and unaffected by this method call.
     *
     * @param amountToSubtract  the period to subtract, not null
     * @return a {@code Period} based on this period with the requested period subtracted, not null
     * @throws ArithmeticException if numeric overflow occurs
     */
    ChronoPeriod minus(ChronoPeriod amountToSubtract);

    //-----------------------------------------------------------------------
    /**
     * Returns a new instance with each amount in this period in this period
     * multiplied by the specified scalar.
     * <p>
     * This returns a period with each supported unit individually multiplied.
     * For example, a period of "2 years, -3 months and 4 days" multiplied by
     * 3 will return "6 years, -9 months and 12 days".
     * No normalization is performed.
     *
     * @param scalar  the scalar to multiply by, not null
     * @return a {@code Period} based on this period with the amounts multiplied
     *  by the scalar, not null
     * @throws ArithmeticException if numeric overflow occurs
     */
    ChronoPeriod multipliedBy(int scalar);

    /**
     * Returns a new instance with each amount in this period negated.
     * <p>
     * This returns a period with each supported unit individually negated.
     * For example, a period of "2 years, -3 months and 4 days" will be
     * negated to "-2 years, 3 months and -4 days".
     * No normalization is performed.
     *
     * @return a {@code Period} based on this period with the amounts negated, not null
     * @throws ArithmeticException if numeric overflow occurs, which only happens if
     *  one of the units has the value {@code Long.MIN_VALUE}
     */
    default ChronoPeriod negated() {
        return multipliedBy(-1);
    }

    //-----------------------------------------------------------------------
    /**
     * Returns a copy of this period with the amounts of each unit normalized.
     * <p>
     * The process of normalization is specific to each calendar system.
     * For example, in the ISO calendar system, the years and months are
     * normalized but the days are not, such that "15 months" would be
     * normalized to "1 year and 3 months".
     * <p>
     * This instance is immutable and unaffected by this method call.
     *
     * @return a {@code Period} based on this period with the amounts of each
     *  unit normalized, not null
     * @throws ArithmeticException if numeric overflow occurs
     */
    ChronoPeriod normalized();

    //-------------------------------------------------------------------------
    /**
     * Adds this period to the specified temporal object.
     * <p>
     * This returns a temporal object of the same observable type as the input
     * with this period added.
     * <p>
     * In most cases, it is clearer to reverse the calling pattern by using
     * {@link Temporal#plus(TemporalAmount)}.
     * <pre>
     *   // these two lines are equivalent, but the second approach is recommended
     *   dateTime = thisPeriod.addTo(dateTime);
     *   dateTime = dateTime.plus(thisPeriod);
     * </pre>
     * <p>
     * The specified temporal must have the same chronology as this period.
     * This returns a temporal with the non-zero supported units added.
     * <p>
     * This instance is immutable and unaffected by this method call.
     *
     * @param temporal  the temporal object to adjust, not null
     * @return an object of the same type with the adjustment made, not null
     * @throws DateTimeException if unable to add
     * @throws ArithmeticException if numeric overflow occurs
     */
    @Override
    Temporal addTo(Temporal temporal);

    /**
     * Subtracts this period from the specified temporal object.
     * <p>
     * This returns a temporal object of the same observable type as the input
     * with this period subtracted.
     * <p>
     * In most cases, it is clearer to reverse the calling pattern by using
     * {@link Temporal#minus(TemporalAmount)}.
     * <pre>
     *   // these two lines are equivalent, but the second approach is recommended
     *   dateTime = thisPeriod.subtractFrom(dateTime);
     *   dateTime = dateTime.minus(thisPeriod);
     * </pre>
     * <p>
     * The specified temporal must have the same chronology as this period.
     * This returns a temporal with the non-zero supported units subtracted.
     * <p>
     * This instance is immutable and unaffected by this method call.
     *
     * @param temporal  the temporal object to adjust, not null
     * @return an object of the same type with the adjustment made, not null
     * @throws DateTimeException if unable to subtract
     * @throws ArithmeticException if numeric overflow occurs
     */
    @Override
    Temporal subtractFrom(Temporal temporal);

    //-----------------------------------------------------------------------
    /**
     * Checks if this period is equal to another period, including the chronology.
     * <p>
     * Compares this period with another ensuring that the amount and chronology are the same.
     * To be equal, the years, months and days units must be individually equal.
     * Note that this means that a period of "15 Months" is not equal to a period
     * of "1 Year and 3 Months".
     *
     * @param obj  the object to check, null returns false
     * @return true if this is equal to the other period
     */
    @Override
    boolean equals(Object obj);

    /**
     * A hash code for this period.
     *
     * @return a suitable hash code
     */
    @Override
    int hashCode();

    //-----------------------------------------------------------------------
    /**
     * Outputs this period as a {@code String}.
     * <p>
     * The output will include the three period fields.
     *
     * @return a string representation of this period, not null
     */
    @Override
    String toString();

}
RogerRiggs commented 11 years ago

Comments:

Overall it probably makes sense, prevents some coding errors and clarifies the model. It will require additional tests to be written.

RogerRiggs commented 11 years ago

JBS mirror issue is 8023762.

jodastephen commented 11 years ago

I don't think it can implement Comparable. The value could be P1Y3M5432D. The days cannot be normalized into months. Thus, comparable would always be a guess, which doesn't seem worth it.

The trouble with getChronology() is that it means that TemporalAmount has no way of getting the full state of the amount object for a period. Duration does not need a chronology, so if you added a getChronology() method to TemporalAmount you'd have to return null from Duration. After thinking it over I think returning null is wrong here, and we'll just have to live with the TemporalAmount limitation.

jodastephen commented 11 years ago

Patch awaiting approval: https://gist.github.com/jodastephen/6392028

RogerRiggs commented 11 years ago

Looks fine, I left a few comments on the patch itself.

jodastephen commented 11 years ago

Just noting that `LocalTime does not have a chronology.

jodastephen commented 11 years ago

Fixed by http://hg.openjdk.java.net/threeten/threeten/jdk/rev/c262597cae74

RogerRiggs commented 11 years ago

It looks like a find and replace went wrong; the javadoc has incorrect markup.

/ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:618: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:618: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:707: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:707: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:981: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/Period.java:981: warning - @Period is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/Chronology.java:1218: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/Chronology.java:1218: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag. /ws/threeten-hg2/jdk/src/share/classes/java/time/chrono/ChronoLocalDate.java:624: warning - @ChronoPeriod is an unknown tag.

jodastephen commented 11 years ago

http://hg.openjdk.java.net/threeten/threeten/jdk/rev/0d78450f7128