PLANNER-549 TemporalValueRange peer review: incrementUnit field name …

…is ambiguous (ename it to incrementUnitType) + javadocs + add amount modulus fail fast

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/valuerange/buildin/bigdecimal/BigDecimalValueRange.java

@@ -31,28 +31,27 @@ public class BigDecimalValueRange extends AbstractCountableValueRange<BigDecimal
     private final BigDecimal from;
     private final BigDecimal to;
     private final BigDecimal incrementUnit;
-    private final int scale;
 
     /**
      * All parameters must have the same {@link BigDecimal#scale()}.
-     * @param from inclusive minimum
-     * @param to exclusive maximum, {@code >= from}
+     * @param from never null, inclusive minimum
+     * @param to never null, exclusive maximum, {@code >= from}
      */
     public BigDecimalValueRange(BigDecimal from, BigDecimal to) {
         this(from, to, BigDecimal.valueOf(1L, from.scale()));
     }
 
     /**
      * All parameters must have the same {@link BigDecimal#scale()}.
-     * @param from inclusive minimum
-     * @param to exclusive maximum, {@code >= from}
-     * @param incrementUnit {@code > 0}
+     * @param from never null, inclusive minimum
+     * @param to never null, exclusive maximum, {@code >= from}
+     * @param incrementUnit never null, {@code > 0}
      */
     public BigDecimalValueRange(BigDecimal from, BigDecimal to, BigDecimal incrementUnit) {
         this.from = from;
         this.to = to;
         this.incrementUnit = incrementUnit;
-        scale = from.scale();
+        int scale = from.scale();
         if (scale != to.scale()) {
             throw new IllegalArgumentException("The " + getClass().getSimpleName()
                     + " cannot have a to (" + to + ") scale (" + to.scale()

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/valuerange/buildin/biginteger/BigIntegerValueRange.java

@@ -32,17 +32,17 @@ public class BigIntegerValueRange extends AbstractCountableValueRange<BigInteger
     private final BigInteger incrementUnit;
 
     /**
-     * @param from inclusive minimum
-     * @param to exclusive maximum, {@code >= from}
+     * @param from never null, inclusive minimum
+     * @param to never null, exclusive maximum, {@code >= from}
      */
     public BigIntegerValueRange(BigInteger from, BigInteger to) {
         this(from, to, BigInteger.valueOf(1L));
     }
 
     /**
-     * @param from inclusive minimum
-     * @param to exclusive maximum, {@code >= from}
-     * @param incrementUnit {@code > 0}
+     * @param from never null, inclusive minimum
+     * @param to never null, exclusive maximum, {@code >= from}
+     * @param incrementUnit never null, {@code > 0}
      */
     public BigIntegerValueRange(BigInteger from, BigInteger to, BigInteger incrementUnit) {
         this.from = from;

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/valuerange/buildin/temporal/TemporalValueRange.java

@@ -4,72 +4,75 @@
 import org.optaplanner.core.impl.domain.valuerange.util.ValueRangeIterator;
 import org.optaplanner.core.impl.solver.random.RandomUtils;
 
+import java.math.BigInteger;
 import java.time.temporal.Temporal;
+import java.time.temporal.TemporalAmount;
 import java.time.temporal.TemporalUnit;
 import java.util.Iterator;
 import java.util.NoSuchElementException;
 import java.util.Random;
 
-/**
- * Created by kevin on 08.04.2016.
- */
 public class TemporalValueRange extends AbstractCountableValueRange<Temporal> {
 
     private final Temporal from;
     private final Temporal to;
-    private final long incrementAmount;
-    private final TemporalUnit incrementUnit;
+    /** We could not use a {@link TemporalAmount} as {@code incrementUnit} due to lack of calculus functions. */
+    private final long incrementUnitAmount;
+    private final TemporalUnit incrementUnitType;
 
     /**
-     * @param from inclusive minimum
-     * @param to exclusive maximum, {@code >= from}
-     * @param incrementAmount {@code > 0}
-     * @param incrementUnit depends on the supported units of {from} and {to}
+     * @param from never null, inclusive minimum
+     * @param to never null, exclusive maximum, {@code >= from}
+     * @param incrementUnitAmount {@code > 0}
+     * @param incrementUnitType never null, must be {@link Temporal#isSupported(TemporalUnit) supported} by {@code from} and {@code to}
      */
-    public TemporalValueRange(Temporal from, Temporal to, long incrementAmount, TemporalUnit incrementUnit) {
+    public TemporalValueRange(Temporal from, Temporal to, long incrementUnitAmount, TemporalUnit incrementUnitType) {
         this.from = from;
         this.to = to;
-        this.incrementAmount = incrementAmount;
-        this.incrementUnit = incrementUnit;
+        this.incrementUnitAmount = incrementUnitAmount;
+        this.incrementUnitType = incrementUnitType;
 
-        if (from == null || to == null || incrementUnit == null) {
+        if (from == null || to == null || incrementUnitType == null) {
             throw new IllegalArgumentException("The " + getClass().getSimpleName()
-                    + " must have from (" + from + "),  to (" + to + "), incrementUnit (" + incrementUnit + ") which are not null.");
+                    + " must have a from (" + from + "),  to (" + to + ") and incrementUnitType (" + incrementUnitType
+                    + ") that are not null.");
         }
-
-        if (incrementAmount <= 0) {
+        if (incrementUnitAmount <= 0) {
             throw new IllegalArgumentException("The " + getClass().getSimpleName()
-                    + " must have strictly positive incrementAmount (" + incrementAmount + ").");
+                    + " must have strictly positive incrementUnitAmount (" + incrementUnitAmount + ").");
         }
-
-        if (!from.isSupported(incrementUnit) || !to.isSupported(incrementUnit)) {
+        if (!from.isSupported(incrementUnitType) || !to.isSupported(incrementUnitType)) {
             throw new IllegalArgumentException("The " + getClass().getSimpleName()
-                    + " must have a incrementUnit which is supported by from/to (" + incrementUnit + ").");
+                    + " must have a incrementUnitType (" + incrementUnitType
+                    + ") that is supported by its from (" + from + ") and to (" + to + ").");
         }
-
-        if (from.until(to, incrementUnit) < 0) {
+        long space = from.until(to, incrementUnitType);
+        if (space < 0) {
             throw new IllegalArgumentException("The " + getClass().getSimpleName()
                     + " cannot have a from (" + from + ") which is strictly higher than its to (" + to + ").");
         }
 
-
-    }
-
-    private long getDuration() {
-        return from.until(to, incrementUnit);
+        // Fail fast if there's a remainder on amount (to be consistent with other value ranges)
+        // Do not fail fast if there's a remainder on type: what is the remainder in months between 31-JAN and 1-MAR?
+        if (space % incrementUnitAmount > 0) {
+            throw new IllegalArgumentException("The " + getClass().getSimpleName()
+                    + " 's incrementUnitAmount (" + incrementUnitAmount
+                    + ") must fit an integer number of times in the space (" + space
+                    + ") between from (" + from + ") and to (" + to + ").");
+        }
     }
 
     @Override
     public long getSize() {
-        return (getDuration() / incrementAmount);
+        return (from.until(to, incrementUnitType) / incrementUnitAmount);
     }
 
     @Override
     public Temporal get(long index) {
         if (index >= getSize() || index < 0) {
             throw new IndexOutOfBoundsException();
         }
-        return from.plus(index * incrementAmount, incrementUnit);
+        return from.plus(index * incrementUnitAmount, incrementUnitType);
     }
 
     @Override
@@ -83,35 +86,35 @@ private class OriginalTemporalValueRangeIterator extends ValueRangeIterator<Temp
 
         @Override
         public boolean hasNext() {
-            return upcoming.until(to, incrementUnit) >= incrementAmount;
+            return upcoming.until(to, incrementUnitType) >= incrementUnitAmount;
         }
 
         @Override
         public Temporal next() {
-            if (upcoming.until(to, incrementUnit) < 0) {
+            if (upcoming.until(to, incrementUnitType) < 0) {
                 throw new NoSuchElementException();
             }
 
             Temporal next = upcoming;
-            upcoming = upcoming.plus(incrementAmount, incrementUnit);
+            upcoming = upcoming.plus(incrementUnitAmount, incrementUnitType);
             return next;
         }
+
     }
 
     @Override
     public boolean contains(Temporal value) {
-        if (value == null || !value.isSupported(incrementUnit)) {return false;}
+        if (value == null || !value.isSupported(incrementUnitType)) {return false;}
 
         // long delta = ...
         // return from.until(value, incrementUnit) >= 0 && to.until(value, incrementUnit) < 0 && delta == 0;
 
         for (long i = 0; i < getSize(); i++) {
-            Temporal temp = get(i);
-            if (value.equals(temp)) {
+            Temporal temporal = get(i);
+            if (value.equals(temporal)) {
                 return true;
             }
         }
-
         return false;
     }
 
@@ -139,6 +142,7 @@ public Temporal next() {
             long index = RandomUtils.nextLong(workingRandom, size);
             return get(index);
         }
+
     }
 
 }

optaplanner-core/src/test/java/org/optaplanner/core/impl/domain/valuerange/buildin/temporal/TemporalValueRangeTest.java

@@ -4,6 +4,7 @@
 
 import java.time.LocalDate;
 import java.time.LocalDateTime;
+import java.time.Period;
 import java.time.temporal.ChronoUnit;
 import java.util.Random;
 
@@ -13,27 +14,29 @@
 import static org.mockito.Mockito.when;
 import static org.optaplanner.core.impl.testdata.util.PlannerAssert.*;
 
-/**
- * Created by kevin on 08.04.2016.
- */
 public class TemporalValueRangeTest {
 
     @Test
-    public void getSize() {
-        // LocalDate
+    public void getSizeForLocalDate() {
         LocalDate from = LocalDate.of(2016, 7, 1);
         LocalDate to = LocalDate.of(2016, 7, 8);
         assertEquals(7L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
-        assertEquals(3L, new TemporalValueRange(from, to, 2, ChronoUnit.DAYS).getSize());
-        assertEquals(0L, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).getSize());
+        assertEquals(1L, new TemporalValueRange(from, to, 7, ChronoUnit.DAYS).getSize());
+        assertEquals(1L, new TemporalValueRange(from, to, 1, ChronoUnit.WEEKS).getSize());
 
         from = LocalDate.of(2016, 7, 7);
         to = LocalDate.of(2016, 7, 17);
         assertEquals(10L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
         assertEquals(5L, new TemporalValueRange(from, to, 2, ChronoUnit.DAYS).getSize());
-        assertEquals(1L, new TemporalValueRange(from, to, 8, ChronoUnit.DAYS).getSize());
+        assertEquals(2L, new TemporalValueRange(from, to, 5, ChronoUnit.DAYS).getSize());
         assertEquals(0L, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).getSize());
 
+        from = LocalDate.of(2016, 1, 31);
+        to = LocalDate.of(2016, 3, 1); // exactly 1 month later
+        assertEquals(30L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
+        assertEquals(15L, new TemporalValueRange(from, to, 2, ChronoUnit.DAYS).getSize());
+        assertEquals(1L, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).getSize());
+
         from = LocalDate.of(2016, 1, 1);
         to = LocalDate.of(2016, 7, 17);
         assertEquals(198L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
@@ -48,12 +51,14 @@ public void getSize() {
 
         from = LocalDate.of(1960, 12, 24);
         to = LocalDate.of(2050, 7, 7);
-        assertEquals( 32702L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
-        assertEquals( 16351L, new TemporalValueRange(from, to, 2, ChronoUnit.DAYS).getSize());
+        assertEquals(32702L, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).getSize());
+        assertEquals(16351L, new TemporalValueRange(from, to, 2, ChronoUnit.DAYS).getSize());
         assertEquals(1074L, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).getSize());
         assertEquals(89L, new TemporalValueRange(from, to, 1, ChronoUnit.YEARS).getSize());
+    }
 
-        // LocalDateTime
+    @Test
+    public void getSizeForLocalDateTime() {
         LocalDateTime fromTime = LocalDateTime.of(2016, 7, 7, 7, 7, 7);
         LocalDateTime toTime = LocalDateTime.of(2016, 7, 7, 7, 7, 8);
         assertEquals(0L, new TemporalValueRange(fromTime, toTime, 1, ChronoUnit.MONTHS).getSize());
@@ -72,8 +77,7 @@ public void getSize() {
     }
 
     @Test
-    public void get() {
-        // LocalDate
+    public void getForLocalDate() {
         LocalDate from = LocalDate.of(2016, 7, 1);
         LocalDate to = LocalDate.of(2016, 7, 8);
         assertEquals(LocalDate.of(2016, 7, 1), new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).get(0));
@@ -89,8 +93,10 @@ public void get() {
         to = LocalDate.of(2016, 1, 1);
         assertEquals(LocalDate.of(1992, 1, 1), new TemporalValueRange(from, to, 1, ChronoUnit.YEARS).get(0));
         assertEquals(LocalDate.of(2015, 1, 1), new TemporalValueRange(from, to, 1, ChronoUnit.YEARS).get(23));
+    }
 
-        // LocalDateTime
+    @Test
+    public void getForLocalDateTime() {
         LocalDateTime fromTime = LocalDateTime.of(2016, 1, 1, 1, 1, 1);
         LocalDateTime toTime = LocalDateTime.of(2016, 7, 7, 7, 7, 7);
         assertEquals(LocalDateTime.of(2016, 1, 1, 1, 1, 1), new TemporalValueRange(fromTime, toTime, 1, ChronoUnit.SECONDS).get(0));
@@ -113,8 +119,7 @@ public void get() {
     }
 
     @Test
-    public void contains() {
-        // LocalDate
+    public void containsForLocalDate() {
         LocalDate from = LocalDate.of(2016, 7, 1);
         LocalDate to = LocalDate.of(2016, 9, 8);
         assertEquals(false, new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).contains(LocalDate.of(2016, 6, 30)));
@@ -129,8 +134,10 @@ public void contains() {
         assertEquals(true, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).contains(LocalDate.of(2016, 8, 1)));
         assertEquals(false, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).contains(LocalDate.of(2016, 9, 1)));
         assertEquals(false, new TemporalValueRange(from, to, 1, ChronoUnit.MONTHS).contains(LocalDate.of(2016, 7, 7)));
+    }
 
-        // LocalDateTime
+    @Test
+    public void containsForLocalDateTime() {
         LocalDateTime fromTime = LocalDateTime.of(2016, 7, 7, 1, 1, 1);
         LocalDateTime toTime = LocalDateTime.of(2016, 7, 7, 7, 7, 7);
         assertEquals(false, new TemporalValueRange(fromTime, toTime, 1, ChronoUnit.HOURS).contains(LocalDateTime.of(2016, 7, 6, 23, 59, 59)));
@@ -150,8 +157,7 @@ public void contains() {
     }
 
     @Test
-    public void createOriginalIterator() {
-        // LocalDate
+    public void createOriginalIteratorForLocalDate() {
         LocalDate from = LocalDate.of(2016, 7, 1);
         LocalDate to = LocalDate.of(2016, 7, 10);
 
@@ -190,8 +196,10 @@ public void createOriginalIterator() {
                 LocalDate.of(2000, 9, 3),
                 LocalDate.of(2001, 9, 3),
                 LocalDate.of(2002, 9, 3));
+    }
 
-        // LocalDateTime
+    @Test
+    public void createOriginalIteratorForLocalDateTime() {
         LocalDateTime fromTime = LocalDateTime.of(2016, 7, 1, 4, 5, 12);
         LocalDateTime toTime = LocalDateTime.of(2016, 7, 3, 12, 12, 17);
 
@@ -212,7 +220,7 @@ public void createOriginalIterator() {
     }
 
     @Test
-    public void createRandomIterator() {
+    public void createRandomIteratorForLocalDate() {
         Random workingRandom = mock(Random.class);
 
         LocalDate from = LocalDate.of(2016, 7, 1);
@@ -225,15 +233,19 @@ public void createRandomIterator() {
         assertElementsOfIterator(new TemporalValueRange(from, to, 1, ChronoUnit.DAYS).createRandomIterator(workingRandom),
                 LocalDate.of(2016, 7, 1));
         when(workingRandom.nextInt(anyInt())).thenReturn(1, 0);
-        assertElementsOfIterator(new TemporalValueRange(from, to, 4, ChronoUnit.DAYS).createRandomIterator(workingRandom),
-                LocalDate.of(2016, 7, 5));
+        assertElementsOfIterator(new TemporalValueRange(from, to, 5, ChronoUnit.DAYS).createRandomIterator(workingRandom),
+                LocalDate.of(2016, 7, 6));
+    }
 
-        // LocalDateTime
+    @Test
+    public void createRandomIteratorForLocalDateTime() {
+        Random workingRandom = mock(Random.class);
         LocalDateTime fromTime = LocalDateTime.of(2016, 7, 1, 4, 5, 12);
-        LocalDateTime toTime = LocalDateTime.of(2016, 7, 3, 12, 12, 17);
+        LocalDateTime toTime = LocalDateTime.of(2016, 7, 3, 12, 15, 12);
 
         when(workingRandom.nextInt(anyInt())).thenReturn(3, 0);
         assertElementsOfIterator(new TemporalValueRange(fromTime, toTime, 10, ChronoUnit.MINUTES).createRandomIterator(workingRandom),
                 LocalDateTime.of(2016, 7, 1, 4, 35, 12));
     }
+
 }