Copilot commented on code in PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#discussion_r4264629193
##########
src/main/java/org/apache/commons/lang3/RandomUtils.java:
##########
@@ -345,7 +345,9 @@ public double randomDouble(final double startInclusive,
final double endExclusiv
if (startInclusive == endExclusive) {
return startInclusive;
}
- return startInclusive + (endExclusive - startInclusive) *
random().nextDouble();
+ final double result = startInclusive + (endExclusive - startInclusive)
* random().nextDouble();
+ // Rounding in the scaling above can reach endExclusive; clamp like
java.util.Random#nextDouble(double, double).
Review Comment:
The comment references `java.util.Random#nextDouble(double, double)` /
`nextFloat(float, float)`, which only exist in newer JDKs (not in Java 8). If
this project still targets older Java versions, consider rewording to avoid
pointing at APIs that don’t exist for all contributors (e.g., describe the
behavior without naming the overload, or reference the JDK implementation
conceptually).
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,33 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round
the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive,
RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
There are two test methods with the same name
(`testNextDoubleExclusiveUpperBound`) differentiated only by their parameter
list. This can make test reports and IDE navigation ambiguous. Consider
renaming one (e.g., `testRandomDoubleExclusiveUpperBound`) and similarly for
the float tests to clearly distinguish static vs instance behavior.
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,33 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round
the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive,
RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
+
+ /**
+ * Test a double range one ULP wide. A previous implementation could round
the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @ParameterizedTest
+ @MethodSource("randomProvider")
+ void testNextDoubleExclusiveUpperBound(final RandomUtils ru) {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, ru.randomDouble(startInclusive,
endExclusive));
+ }
+ }
Review Comment:
There are two test methods with the same name
(`testNextDoubleExclusiveUpperBound`) differentiated only by their parameter
list. This can make test reports and IDE navigation ambiguous. Consider
renaming one (e.g., `testRandomDoubleExclusiveUpperBound`) and similarly for
the float tests to clearly distinguish static vs instance behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
