aherbert commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476704947
##########
File path:
commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
##########
@@ -622,6 +622,16 @@ void testMath1261() {
assertFraction(1, Integer.MAX_VALUE, b.divide(2));
}
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () ->
Fraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () ->
Fraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () ->
Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
Add the case for 1 / 2 as well.
##########
File path:
commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new
FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
+ this.denominator.pow(exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent / 2;
+ final BigInteger newDenominator = denominator.pow(-temp);
+ final BigInteger newNumerator = numerator.pow(-temp);
+ return new BigFraction(newDenominator.multiply(newDenominator),
Review comment:
Why not the same as Fraction:
```java
return new Fraction(denominator.pow(Integer.MAX_VALUE).multiply(denominator),
numerator.pow(Integer.MAX_VALUE).multiply(numerator));
```
##########
File path:
commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new
FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
Review comment:
No need for `this` keyword.
##########
File path:
commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new
FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(ArithmeticUtils.pow(this.denominator,
Integer.MAX_VALUE) * this.denominator,
+ ArithmeticUtils.pow(this.numerator, Integer.MAX_VALUE) *
this.numerator);
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ return new Fraction(ArithmeticUtils.pow(this.denominator, -exponent),
+ ArithmeticUtils.pow(this.numerator, -exponent));
Review comment:
Formatting should match the original with whitespace to align the code
between lines.
##########
File path:
commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new
FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
Review comment:
No need to `this` keyword
##########
File path:
commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
##########
@@ -778,4 +778,14 @@ void testMath340() {
fractionA.getDenominator().multiply(fractionB.getDenominator()));
Assertions.assertEquals(correctResult, errorResult);
}
+
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () ->
BigFraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () ->
BigFraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () ->
Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
I would add `Fraction.of(1, 2)` as well to test overflow in the
numerator or denominator
##########
File path:
commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
##########
@@ -570,6 +570,7 @@ private CommonTestCases() {}
testCases.add(new BinaryIntOperatorTestCase(0, 1, Integer.MAX_VALUE,
0, 1));
testCases.add(new BinaryIntOperatorTestCase(0, -1, Integer.MAX_VALUE,
0, 1));
+ testCases.add(new BinaryIntOperatorTestCase(1, 1, Integer.MIN_VALUE,
1, 1));
Review comment:
I would add all the cases for completeness:
```
1, 1
1, -1
-1, 1
-1, -1
```
These should all evaluate to 1 / 1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
