#1386 fix nextInt and nextLong with extreme boundaries

e.g. * `randomService.nextInt(Integer.MIN_VALUE, Integer.MAX_VALUE)` * `randomService.nextLong(Long.MIN_VALUE, Long.MAX_VALUE)` P.S. document the legacy behaviour of `nextLong` method (when min == max)
datafaker-net · Oct 21, 2024 · b60ed48 · b60ed48
1 parent c94d690
commit b60ed48
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 28 deletions.
diff --git a/src/main/java/net/datafaker/idnumbers/SingaporeIdNumber.java b/src/main/java/net/datafaker/idnumbers/SingaporeIdNumber.java
@@ -83,9 +83,9 @@ static LocalDate randomBirthDate(BaseProviders faker, Type type) {
         int now = LocalDate.now().getYear();
         return switch (type) {
             case SINGAPOREAN_TWENTIETH_CENTURY,
-                 FOREIGNER_TWENTIETH_CENTURY -> faker.timeAndDate().birthday(now - 1900, now - 1999);
+                 FOREIGNER_TWENTIETH_CENTURY -> faker.timeAndDate().birthday(now - 1999, now - 1900);
             case SINGAPOREAN_TWENTY_FIRST_CENTURY,
-                 FOREIGNER_TWENTY_FIRST_CENTURY -> faker.timeAndDate().birthday(now - 2000, now - 2099);
+                 FOREIGNER_TWENTY_FIRST_CENTURY -> faker.timeAndDate().birthday(Math.max(0, now - 2099), now - 2000);
         };
     }
 

diff --git a/src/main/java/net/datafaker/providers/base/DateAndTime.java b/src/main/java/net/datafaker/providers/base/DateAndTime.java
@@ -322,8 +322,8 @@ public Duration duration(long max, ChronoUnit unit) {
     /**
      * Generates a random Duration between min and max.
      *
-     * @param min  the maximum value
-     * @param max  the minimal value
+     * @param min  the minimum value
+     * @param max  the maximum value
      * @param unit the temporal unit (day or shorter than a day)
      * @return a random Duration between {@code min} inclusive and {@code max} exclusive if {@code max} greater {@code min}.
      * @throws IllegalArgumentException if the {@code unit} is invalid.

diff --git a/src/main/java/net/datafaker/providers/base/TimeAndDate.java b/src/main/java/net/datafaker/providers/base/TimeAndDate.java
@@ -301,8 +301,8 @@ public Duration duration(long max, ChronoUnit unit) {
     /**
      * Generates a random Duration between min and max.
      *
-     * @param min  the maximum value
-     * @param max  the minimal value
+     * @param min  the minimum value
+     * @param max  the maximum value
      * @param unit the temporal unit (day or shorter than a day)
      * @return a random Duration between {@code min} inclusive and {@code max} exclusive if {@code max} greater {@code min}.
      * @throws IllegalArgumentException if the {@code unit} is invalid.

diff --git a/src/main/java/net/datafaker/service/RandomService.java b/src/main/java/net/datafaker/service/RandomService.java
@@ -28,12 +28,17 @@ public int nextInt() {
         return random.nextInt();
     }
 
-    public int nextInt(int n) {
-        return random.nextInt(n);
+    public int nextInt(int maxExclusive) {
+        return random.nextInt(maxExclusive);
     }
 
-    public Integer nextInt(int min, int max) {
-        return random.nextInt(min, max + 1);
+    public Integer nextInt(int minInclusive, int maxInclusive) {
+        if (minInclusive > maxInclusive)
+            throw new IllegalArgumentException("Min (%s) > Max (%s)".formatted(minInclusive, maxInclusive));
+        if (maxInclusive + 1 < 0)
+            return (int) nextLong(minInclusive, maxInclusive);
+
+        return random.nextInt(minInclusive, maxInclusive + 1);
     }
 
     @SuppressWarnings("unused")
@@ -45,23 +50,33 @@ public long nextLong() {
         return random.nextLong();
     }
 
-    // lifted from http://stackoverflow.com/questions/2546078/java-random-long-number-in-0-x-n-range
-    public long nextLong(long n) {
-        if (n <= 0) {
-            throw new IllegalArgumentException("bound must be positive: " + n);
+    public long nextLong(long maxInclusive) {
+        if (maxInclusive <= 0) {
+            throw new IllegalArgumentException("bound must be positive: " + maxInclusive);
         }
 
         long bits, val;
         do {
             long randomLong = random.nextLong();
             bits = (randomLong << 1) >>> 1;
-            val = bits % n;
-        } while (bits - val + (n - 1) < 0L);
+            val = bits % maxInclusive;
+        } while (bits - val + (maxInclusive - 1) < 0L);
         return val;
     }
 
+    /**
+     * A random long value within given range.
+     * If {@code min == max} then method always returns {@code min}.
+     * Otherwise, {@code max} is exclusive.
+     *
+     * @param min lower bound (inclusive)
+     * @param max upper bound (exclusive in most case)
+     * @return a random long value between {@code min} and {@code max}
+     */
     public long nextLong(long min, long max) {
-        return min + (long) (nextDouble() * (max - min));
+        return min == max ?
+            min :
+            random.nextLong(min, max);
     }
 
     public double nextDouble() {

diff --git a/src/test/java/net/datafaker/providers/base/TimeAndDateTest.java b/src/test/java/net/datafaker/providers/base/TimeAndDateTest.java
@@ -173,19 +173,23 @@ void durationTest(long minValue, long maxValue, ChronoUnit unit) {
             .as("Duration must be equal or greater than min value")
             .isLessThanOrEqualTo(generated);
         assertThat(max.compareTo(generated) > 0 || minValue >= maxValue && max.equals(generated))
-            .as("Duration must be lower than max value").isTrue();
+            .as(() -> "Duration (%s) must be lower than max value (%s) / min value (%s)".formatted(generated, max, min)).isTrue();
     }
 
     @ParameterizedTest
     @MethodSource("generateDurationsWithMaxOnly")
     void durationTest(long maxValue, ChronoUnit unit) {
         Duration generated = timeAndDate.duration(maxValue, unit);
-        Duration max = Duration.of(maxValue, unit);
-        assertThat(max.compareTo(generated) > 0 || maxValue == 0)
-            .as("Duration must be lower than max value")
-            .isTrue();
+        assertThat(generated)
+            .as(() -> "Duration (%s) must be lower than max value (%s %s)".formatted(generated, maxValue, unit))
+            .isLessThan(Duration.of(maxValue, unit));
     }
 
+    @Test
+    void durationIsZero_ifMaxIsZero() {
+        assertThat(timeAndDate.duration(0, ChronoUnit.DAYS))
+            .isEqualTo(Duration.of(0, ChronoUnit.DAYS));
+    }
 
     @ParameterizedTest
     @MethodSource("generatePeriod")
@@ -196,7 +200,6 @@ void maxLessThanMinPeriod(Period min, Period max) {
 
     private static Stream<Arguments> generateDurationsWithMaxOnly() {
         return Stream.of(
-            Arguments.of(0, ChronoUnit.DAYS),
             Arguments.of(100, ChronoUnit.DAYS),
             Arguments.of(456, ChronoUnit.HOURS),
             Arguments.of(43, ChronoUnit.MINUTES),

diff --git a/src/test/java/net/datafaker/service/RandomNumbersTest.java b/src/test/java/net/datafaker/service/RandomNumbersTest.java
@@ -0,0 +1,39 @@
+package net.datafaker.service;
+
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.Test;
+
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.function.Supplier;
+
+import static org.assertj.core.api.Assertions.*;
+
+class RandomNumbersTest {
+    private final RandomService randomService = new RandomService();
+
+    @Test
+    void nextInt_minInclusive_maxExclusive() {
+        assertThat(all(() -> randomService.nextLong(3))).containsExactly(0L, 1L, 2L);
+        assertThat(all(() -> randomService.nextLong(2, 4))).containsExactly(2L, 3L);
+        assertThat(all(() -> randomService.nextInt(3))).containsExactly(0, 1, 2);
+
+        // inclusive: see https://github.com/datafaker-net/datafaker/issues/1395
+        assertThat(all(() -> randomService.nextInt(2, 4))).containsExactly(2, 3, 4);
+    }
+
+    @RepeatedTest(100)
+    void nextDouble() {
+        assertThat(randomService.nextDouble(2, 3)).isGreaterThanOrEqualTo(2);
+        assertThat(randomService.nextDouble(2, 3)).isStrictlyBetween(1.9999, 3.0);
+    }
+
+    private <T extends Number & Comparable<T>> SortedSet<T> all(Supplier<T> lambda) {
+        SortedSet<T> result = new TreeSet<>();
+        for (int i = 0; i < 1000; i++) {
+            result.add(lambda.get());
+        }
+        return result;
+    }
+
+}
diff --git a/src/test/java/net/datafaker/service/RandomServiceTest.java b/src/test/java/net/datafaker/service/RandomServiceTest.java
@@ -2,6 +2,7 @@
 
 import net.datafaker.AbstractFakerTest;
 import org.assertj.core.api.Condition;
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -86,12 +87,26 @@ void predictableRandomRange() {
         assertThat(f1).isEqualTo(0.41291267F);
 
         assertThat(l1).isEqualTo(1092083446069765248L);
-        assertThat(l2).isOne();
-        assertThat(l3).isEqualTo(836L);
+        assertThat(l2).isEqualTo(1L);
+        assertThat(l3).isEqualTo(538L);
 
         assertThat(b).isFalse();
     }
 
+    @Test
+    void equalMinAndMax() {
+        RandomService randomService = new RandomService();
+        assertThat(randomService.nextInt(42, 42)).isEqualTo(42);
+    }
+
+    @RepeatedTest(10)
+    void extremeIntegerValues() {
+        RandomService randomService = new RandomService();
+        assertThat(randomService.nextInt(1, Integer.MAX_VALUE)).isBetween(1, Integer.MAX_VALUE);
+        assertThat(randomService.nextInt(Integer.MIN_VALUE, 0)).isBetween(Integer.MIN_VALUE, 0);
+        assertThat(randomService.nextInt(Integer.MIN_VALUE, Integer.MAX_VALUE)).isBetween(Integer.MIN_VALUE, Integer.MAX_VALUE);
+    }
+
     @ParameterizedTest
     @MethodSource("randomServiceProvider")
     void testDoubleInRange(RandomService randomService) {
@@ -105,13 +120,19 @@ void testDoubleInRange(RandomService randomService) {
     @ParameterizedTest
     @MethodSource("randomServiceProvider")
     void testLongInRange(RandomService randomService) {
-        final Condition<Long> lessThanOrEqual = new Condition<>(t -> t <= 5_000_000_000L, "should be less than or equal 5_000_000_000L");
-        final Condition<Long> greaterThanOrEqual = new Condition<>(t -> t >= -5_000_000_000L, "should be greater than or equal -5_000_000_000L");
         for (int i = 1; i < 1_000; i++) {
-            assertThat(randomService.nextLong(-5_000_000_000L, 5_000_000_000L)).is(allOf(lessThanOrEqual, greaterThanOrEqual));
+            assertThat(randomService.nextLong(-5_000_000_000L, 5_000_000_000L)).isBetween(-5_000_000_000L, 5_000_000_000L);
         }
     }
 
+    @RepeatedTest(10)
+    void extremeLongValues() {
+        RandomService randomService = new RandomService();
+        assertThat(randomService.nextLong(0, Long.MAX_VALUE - 1)).isBetween(0L, Long.MAX_VALUE - 1);
+        assertThat(randomService.nextLong(Long.MIN_VALUE, 0)).isBetween(Long.MIN_VALUE, 0L);
+        assertThat(randomService.nextLong(Long.MIN_VALUE, Long.MAX_VALUE)).isBetween(Long.MIN_VALUE, Long.MAX_VALUE);
+    }
+
     @ParameterizedTest
     @MethodSource("randomServiceProvider")
     void testHex(RandomService randomService) {