[GitHub] [commons-numbers] aherbert commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow

[GitBox <gi...@apache.org>]

aherbert commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476624765



##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonsLangPortedFractionTest.java
##########
@@ -0,0 +1,1110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.numbers.fraction;
+
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test cases for the {@link Fraction} class
+ */
+public class CommonsLangPortedFractionTest {

Review comment:
       This class should not be in this PR.
   
   You should update FractionTest to add assertions that fail with the current code. See the comments in the Jira ticket.
   
   A similar change is warranted for BigFraction.
   

##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
      */
     @Override
     public Fraction pow(final int exponent) {
+        if (exponent == 1) {
+            return this;
+        }
         if (exponent == 0) {
             return ONE;
         }
-        if (isZero()) {
-            return ZERO;
+        if (exponent == -1) {
+            return this.reciprocal();
         }
-
-        if (exponent < 0) {
-            return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
-                                ArithmeticUtils.pow(numerator,   -exponent));
+        if (this.denominator == 0) {

Review comment:
       The private constructor should make it impossible to have a denominator of zero. See if you can write a test that hits this code execution path.
   
   This functionality is better served by the current logic that checks if (isZero()).

##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
      */
     @Override
     public Fraction pow(final int exponent) {
+        if (exponent == 1) {
+            return this;
+        }
         if (exponent == 0) {
             return ONE;
         }
-        if (isZero()) {
-            return ZERO;
+        if (exponent == -1) {
+            return this.reciprocal();
         }
-
-        if (exponent < 0) {
-            return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
-                                ArithmeticUtils.pow(numerator,   -exponent));
+        if (this.denominator == 0) {
+            if (exponent > 0) {
+                throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+            } else {
+                return ZERO;
+            }
+        }
+        if (this.numerator == 0) {
+            if (exponent < 0) {
+                throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+            } else {
+                return ZERO;
+            }
         }
-        return new Fraction(ArithmeticUtils.pow(numerator,   exponent),
-                            ArithmeticUtils.pow(denominator, exponent));
+        if (exponent > 0) {
+            return new Fraction(
+                    ArithmeticUtils.pow(this.numerator, exponent),
+                    ArithmeticUtils.pow(this.denominator, exponent)
+            );
+        }
+        if (exponent == Integer.MIN_VALUE) {
+            return new Fraction(
+                    ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,

Review comment:
       A raw multiplication here is a bad idea. You should at least use Math.multipyExact. For this extreme edge case I think some variant of the following simpler logic is clearer:
   ```java
   if (exponent == Integer.MIN_VALUE) {
         // MIN_VALUE can't be negated
         final int temp = exponent / 2;
         return new Fraction(ArithmeticUtils.pow(denominator, -temp),
                             ArithmeticUtils.pow(numerator,   -temp)).pow(2);
   }
   ```




----------------------------------------------------------------
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:
users@infra.apache.org