search

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

TemporalField isDateField() and isTimeField() #296

Closed jodastephen closed 11 years ago

jodastephen commented 11 years ago

More than once, code would have been simpler to write and understand if TemporalField had the methods isDateField() and isTimeField() that are currently only on ChronoField. This came up tonight while fixing bugs in DateTimePrintContext, and the current bug fix really needs this change to be complete.

The methods would also be better named isDateBased() and isTimeBased(), as the "field" part of the name is redundant on a class named "field".

jodastephen commented 11 years ago 
# HG changeset patch
# User scolebourne
# Date 1364779761 -3600
# Node ID 4517b8abe0eb72fb31b47f55a3d0159f4815d321
# Parent  9acd8d0c39dc4a09b61218955acf67bb93775054
Add TemporalField.isDatebased/isTimeBased

Rename methods on ChronoField
Fix bug in DateTimePrintContext using new information

diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/LocalDate.java
--- a/src/share/classes/java/time/LocalDate.java    Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/LocalDate.java    Mon Apr 01 02:29:21 2013 +0100
@@ -528,7 +528,7 @@
     public ValueRange range(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            if (f.isDateField()) {
+            if (f.isDateBased()) {
                 switch (f) {
                     case DAY_OF_MONTH: return ValueRange.of(1, lengthOfMonth());
                     case DAY_OF_YEAR: return ValueRange.of(1, lengthOfYear());
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/LocalDateTime.java
--- a/src/share/classes/java/time/LocalDateTime.java    Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/LocalDateTime.java    Mon Apr 01 02:29:21 2013 +0100
@@ -564,7 +564,7 @@
     public boolean isSupported(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return f.isDateField() || f.isTimeField();
+            return f.isDateBased() || f.isTimeBased();
         }
         return field != null && field.isSupportedBy(this);
     }
@@ -596,7 +596,7 @@
     public ValueRange range(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.range(field) : date.range(field));
+            return (f.isTimeBased() ? time.range(field) : date.range(field));
         }
         return field.rangeRefinedBy(this);
     }
@@ -633,7 +633,7 @@
     public int get(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.get(field) : date.get(field));
+            return (f.isTimeBased() ? time.get(field) : date.get(field));
         }
         return ChronoLocalDateTime.super.get(field);
     }
@@ -665,7 +665,7 @@
     public long getLong(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.getLong(field) : date.getLong(field));
+            return (f.isTimeBased() ? time.getLong(field) : date.getLong(field));
         }
         return field.getFrom(this);
     }
@@ -910,7 +910,7 @@
     public LocalDateTime with(TemporalField field, long newValue) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            if (f.isTimeField()) {
+            if (f.isTimeBased()) {
                 return with(date, time.with(field, newValue));
             } else {
                 return with(date.with(field, newValue), time);
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/LocalTime.java
--- a/src/share/classes/java/time/LocalTime.java    Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/LocalTime.java    Mon Apr 01 02:29:21 2013 +0100
@@ -505,7 +505,7 @@
     @Override
     public boolean isSupported(TemporalField field) {
         if (field instanceof ChronoField) {
-            return ((ChronoField) field).isTimeField();
+            return field.isTimeBased();
         }
         return field != null && field.isSupportedBy(this);
     }
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/OffsetTime.java
--- a/src/share/classes/java/time/OffsetTime.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/OffsetTime.java   Mon Apr 01 02:29:21 2013 +0100
@@ -384,7 +384,7 @@
     @Override
     public boolean isSupported(TemporalField field) {
         if (field instanceof ChronoField) {
-            return ((ChronoField) field).isTimeField() || field == OFFSET_SECONDS;
+            return field.isTimeBased() || field == OFFSET_SECONDS;
         }
         return field != null && field.isSupportedBy(this);
     }
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/chrono/ChronoLocalDate.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDate.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/chrono/ChronoLocalDate.java   Mon Apr 01 02:29:21 2013 +0100
@@ -369,7 +369,7 @@
     @Override
     public default boolean isSupported(TemporalField field) {
         if (field instanceof ChronoField) {
-            return ((ChronoField) field).isDateField();
+            return field.isDateBased();
         }
         return field != null && field.isSupportedBy(this);
     }
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java   Mon Apr 01 02:29:21 2013 +0100
@@ -222,7 +222,7 @@
     public boolean isSupported(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return f.isDateField() || f.isTimeField();
+            return f.isDateBased() || f.isTimeBased();
         }
         return field != null && field.isSupportedBy(this);
     }
@@ -231,7 +231,7 @@
     public ValueRange range(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.range(field) : date.range(field));
+            return (f.isTimeBased() ? time.range(field) : date.range(field));
         }
         return field.rangeRefinedBy(this);
     }
@@ -240,7 +240,7 @@
     public int get(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.get(field) : date.get(field));
+            return (f.isTimeBased() ? time.get(field) : date.get(field));
         }
         return range(field).checkValidIntValue(getLong(field), field);
     }
@@ -249,7 +249,7 @@
     public long getLong(TemporalField field) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            return (f.isTimeField() ? time.getLong(field) : date.getLong(field));
+            return (f.isTimeBased() ? time.getLong(field) : date.getLong(field));
         }
         return field.getFrom(this);
     }
@@ -273,7 +273,7 @@
     public ChronoLocalDateTimeImpl<D> with(TemporalField field, long newValue) {
         if (field instanceof ChronoField) {
             ChronoField f = (ChronoField) field;
-            if (f.isTimeField()) {
+            if (f.isTimeBased()) {
                 return with(date, time.with(field, newValue));
             } else {
                 return with(date.with(field, newValue), time);
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/format/DateTimePrintContext.java
--- a/src/share/classes/java/time/format/DateTimePrintContext.java  Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/format/DateTimePrintContext.java  Mon Apr 01 02:29:21 2013 +0100
@@ -165,7 +165,7 @@
                 // check for date fields other than epoch-day, ignoring case of converting null to ISO
                 if (!(overrideChrono == IsoChronology.INSTANCE && temporalChrono == null)) {
                     for (ChronoField f : ChronoField.values()) {
-                        if (f.isDateField() && temporal.isSupported(f)) {
+                        if (f.isDateBased() && temporal.isSupported(f)) {
                             throw new DateTimeException("Unable to apply override chronology '" + overrideChrono +
                                     "' because the temporal object being formatted contains date fields but" +
                                     " does not represent a whole date: " + temporal);
@@ -184,21 +184,21 @@
         return new TemporalAccessor() {
             @Override
             public boolean isSupported(TemporalField field) {
-                if (effectiveDate != null && field instanceof ChronoField && ((ChronoField) field).isDateField()) {
+                if (effectiveDate != null && field.isDateBased()) {
                     return effectiveDate.isSupported(field);
                 }
                 return temporal.isSupported(field);
             }
             @Override
             public ValueRange range(TemporalField field) {
-                if (effectiveDate != null && field instanceof ChronoField && ((ChronoField) field).isDateField()) {
+                if (effectiveDate != null && field.isDateBased()) {
                     return effectiveDate.range(field);
                 }
                 return temporal.range(field);
             }
             @Override
             public long getLong(TemporalField field) {
-                if (effectiveDate != null && field instanceof ChronoField && ((ChronoField) field).isDateField()) {
+                if (effectiveDate != null && field.isDateBased()) {
                     return effectiveDate.getLong(field);
                 }
                 return temporal.getLong(field);
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/temporal/ChronoField.java
--- a/src/share/classes/java/time/temporal/ChronoField.java Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/temporal/ChronoField.java Mon Apr 01 02:29:21 2013 +0100
@@ -588,19 +588,25 @@
     //-----------------------------------------------------------------------
     /**
      * Checks if this field represents a component of a date.
+     * <p>
+     * Fields from day-of-week to era are date-based.
      *
      * @return true if it is a component of a date
      */
-    public boolean isDateField() {
+    @Override
+    public boolean isDateBased() {
         return ordinal() >= DAY_OF_WEEK.ordinal() && ordinal() <= ERA.ordinal();
     }

     /**
      * Checks if this field represents a component of a time.
+     * <p>
+     * Fields from nano-of-second to am-pm-of-day are time-based.
      *
      * @return true if it is a component of a time
      */
-    public boolean isTimeField() {
+    @Override
+    public boolean isTimeBased() {
         return ordinal() < DAY_OF_WEEK.ordinal();
     }

diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/temporal/IsoFields.java
--- a/src/share/classes/java/time/temporal/IsoFields.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/temporal/IsoFields.java   Mon Apr 01 02:29:21 2013 +0100
@@ -453,6 +453,11 @@
         };

         @Override
+        public boolean isDateBased() {
+            return true;
+        }
+
+        @Override
         public ValueRange rangeRefinedBy(TemporalAccessor temporal) {
             return range();
         }
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/temporal/JulianFields.java
--- a/src/share/classes/java/time/temporal/JulianFields.java    Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/temporal/JulianFields.java    Mon Apr 01 02:29:21 2013 +0100
@@ -232,6 +232,11 @@
         }

         @Override
+        public boolean isDateBased() {
+            return true;
+        }
+
+        @Override
         public ValueRange range() {
             return range;
         }
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/temporal/TemporalField.java
--- a/src/share/classes/java/time/temporal/TemporalField.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/temporal/TemporalField.java   Mon Apr 01 02:29:21 2013 +0100
@@ -145,6 +145,35 @@

     //-----------------------------------------------------------------------
     /**
+     * Checks if this field represents a component of a date.
+     * <p>
+     * A field is date-based if it can be derived from
+     * {@link ChronoField#EPOCH_DAY EPOCH_DAY}.
+     * <p>
+     * The default implementation must return false.
+     *
+     * @return true if it is a component of a date
+     */
+    public default boolean isDateBased() {
+        return false;
+    }
+
+    /**
+     * Checks if this field represents a component of a time.
+     * <p>
+     * A field is date-based if it can be derived from
+     * {@link ChronoField#NANO_OF_DAY NANO_OF_DAY}.
+     * <p>
+     * The default implementation must return false.
+     *
+     * @return true if it is a component of a time
+     */
+    public default boolean isTimeBased() {
+        return false;
+    }
+
+    //-----------------------------------------------------------------------
+    /**
      * Compares the value of this field in two temporal objects.
      * <p>
      * All fields implement {@link Comparator} on {@link TemporalAccessor}.
diff -r 9acd8d0c39dc -r 4517b8abe0eb src/share/classes/java/time/temporal/WeekFields.java
--- a/src/share/classes/java/time/temporal/WeekFields.java  Mon Apr 01 02:00:04 2013 +0100
+++ b/src/share/classes/java/time/temporal/WeekFields.java  Mon Apr 01 02:29:21 2013 +0100
@@ -894,6 +894,11 @@
         }

         @Override
+        public boolean isDateBased() {
+            return true;
+        }
+
+        @Override
         public ValueRange range() {
             return range;
         }
diff -r 9acd8d0c39dc -r 4517b8abe0eb test/java/time/tck/java/time/temporal/TCKIsoFields.java
--- a/test/java/time/tck/java/time/temporal/TCKIsoFields.java   Mon Apr 01 02:00:04 2013 +0100
+++ b/test/java/time/tck/java/time/temporal/TCKIsoFields.java   Mon Apr 01 02:29:21 2013 +0100
@@ -121,6 +121,11 @@
         assertEquals(date.get(IsoFields.DAY_OF_QUARTER), doq);
     }

+    public void test_DOQ_basics() {
+        assertEquals(IsoFields.DAY_OF_QUARTER.isDateBased(), true);
+        assertEquals(IsoFields.DAY_OF_QUARTER.isTimeBased(), false);
+    }
+
     //-----------------------------------------------------------------------
     // QUARTER_OF_YEAR
     //-----------------------------------------------------------------------
@@ -130,6 +135,11 @@
         assertEquals(date.get(IsoFields.QUARTER_OF_YEAR), qoy);
     }

+    public void test_QOY_basics() {
+        assertEquals(IsoFields.QUARTER_OF_YEAR.isDateBased(), true);
+        assertEquals(IsoFields.QUARTER_OF_YEAR.isTimeBased(), false);
+    }
+
     //-----------------------------------------------------------------------
     // parse quarters
     //-----------------------------------------------------------------------
@@ -214,6 +224,11 @@
         assertEquals(date.get(IsoFields.WEEK_OF_WEEK_BASED_YEAR), week);
     }

+    public void test_WOWBY_basics() {
+        assertEquals(IsoFields.WEEK_OF_WEEK_BASED_YEAR.isDateBased(), true);
+        assertEquals(IsoFields.WEEK_OF_WEEK_BASED_YEAR.isTimeBased(), false);
+    }
+
     //-----------------------------------------------------------------------
     // WEEK_BASED_YEAR
     //-----------------------------------------------------------------------
@@ -224,6 +239,11 @@
         assertEquals(date.get(IsoFields.WEEK_BASED_YEAR), wby);
     }

+    public void test_WBY_basics() {
+        assertEquals(IsoFields.WEEK_BASED_YEAR.isDateBased(), true);
+        assertEquals(IsoFields.WEEK_BASED_YEAR.isTimeBased(), false);
+    }
+
     //-----------------------------------------------------------------------
     // parse weeks
     //-----------------------------------------------------------------------
diff -r 9acd8d0c39dc -r 4517b8abe0eb test/java/time/tck/java/time/temporal/TCKJulianFields.java
--- a/test/java/time/tck/java/time/temporal/TCKJulianFields.java    Mon Apr 01 02:00:04 2013 +0100
+++ b/test/java/time/tck/java/time/temporal/TCKJulianFields.java    Mon Apr 01 02:29:21 2013 +0100
@@ -66,6 +66,7 @@
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
 import java.time.temporal.ChronoField;
+import java.time.temporal.IsoFields;
 import java.time.temporal.JulianFields;
 import java.time.temporal.TemporalField;

@@ -125,6 +126,17 @@
         assertSerializable(field);
     }

+    public void test_basics() {
+        assertEquals(JulianFields.JULIAN_DAY.isDateBased(), true);
+        assertEquals(JulianFields.JULIAN_DAY.isTimeBased(), false);
+
+        assertEquals(JulianFields.MODIFIED_JULIAN_DAY.isDateBased(), true);
+        assertEquals(JulianFields.MODIFIED_JULIAN_DAY.isTimeBased(), false);
+
+        assertEquals(JulianFields.RATA_DIE.isDateBased(), true);
+        assertEquals(JulianFields.RATA_DIE.isTimeBased(), false);
+    }
+
     //-----------------------------------------------------------------------
     @Test(dataProvider="samples")
     public void test_samples_get(TemporalField field, LocalDate date, long expected) {
diff -r 9acd8d0c39dc -r 4517b8abe0eb test/java/time/tck/java/time/temporal/TCKWeekFields.java
--- a/test/java/time/tck/java/time/temporal/TCKWeekFields.java  Mon Apr 01 02:00:04 2013 +0100
+++ b/test/java/time/tck/java/time/temporal/TCKWeekFields.java  Mon Apr 01 02:29:21 2013 +0100
@@ -71,6 +71,7 @@
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
 import java.time.temporal.ChronoUnit;
+import java.time.temporal.JulianFields;
 import java.time.temporal.TemporalField;
 import java.time.temporal.ValueRange;
 import java.time.temporal.WeekFields;
@@ -94,6 +95,26 @@
     }

     //-----------------------------------------------------------------------
+    @Test(dataProvider="weekFields")
+    public void test_basics(DayOfWeek firstDayOfWeek, int minDays) {
+        WeekFields week = WeekFields.of(firstDayOfWeek, minDays);
+        assertEquals(week.dayOfWeek().isDateBased(), true);
+        assertEquals(week.dayOfWeek().isTimeBased(), false);
+
+        assertEquals(week.weekOfMonth().isDateBased(), true);
+        assertEquals(week.weekOfMonth().isTimeBased(), false);
+
+        assertEquals(week.weekOfYear().isDateBased(), true);
+        assertEquals(week.weekOfYear().isTimeBased(), false);
+
+        assertEquals(week.weekOfWeekBasedYear().isDateBased(), true);
+        assertEquals(week.weekOfWeekBasedYear().isTimeBased(), false);
+
+        assertEquals(week.weekBasedYear().isDateBased(), true);
+        assertEquals(week.weekBasedYear().isTimeBased(), false);
+    }
+
+    //-----------------------------------------------------------------------
     @Test
     public void test_dayOfWeekField_simpleGet() {
         LocalDate date = LocalDate.of(2000, 1, 10);  // Known to be ISO Monday
RogerRiggs commented 11 years ago

The code seems to assume that isDateBased() == !isTimeBased; but if so then why two methods.

jodastephen commented 11 years ago

Fields like OFFSET_SECONDS and INSTANT_SECONDS will return false from both methods. What matters is whether the field can be derived from EPOCH_DAY/NANO_OF_DAY.

jodastephen commented 11 years ago

Applied in http://hg.openjdk.java.net/threeten/threeten/jdk/rev/a98a9eb86c54