[GitHub] [commons-numbers] aherbert commented on a diff in pull request #129: Refactoring changes

["aherbert (via GitHub)" <gi...@apache.org>]

aherbert commented on code in PR #129:
URL: https://github.com/apache/commons-numbers/pull/129#discussion_r1160488862


##########
commons-numbers-examples/examples-jmh/src/main/java/org/apache/commons/numbers/examples/jmh/gamma/ErfPerformance.java:
##########
@@ -146,6 +146,16 @@ protected double[] createNumbers(SplittableRandom rng) {
      * Contains an array of numbers and the method to compute the error function.
      */
     public abstract static class FunctionData extends NumberData {
+        /** The type of the data.
+         Should be num uniform
+         Declared in ErfPerformance class. **/
+        @Param({NUM_UNIFORM})
+        private String NUM_TYPE;

Review Comment:
   The previous benchmark had a single variable `type` in each `@State` class. By splitting this into two you will cause JMH to run the benchmark with more variables. JMH creates benchmarks as the cross-product of all variables. Thus one of the states will not have `1 * 2` benchmarks where previously it would be 1. This is a waste of resources. It will also create two columns in the results where the two columns have redundant information as child classes use only one of the variables.



##########
commons-numbers-primes/src/main/java/org/apache/commons/numbers/primes/Primes.java:
##########
@@ -81,12 +96,7 @@ public static int nextPrime(int n) {
 
         // prepare entry in the +2, +4 loop:
         // n should not be a multiple of 3
-        final int rem = n % 3;
-        if (0 == rem) { // if n % 3 == 0
-            n += 2; // n % 3 == 2
-        } else if (1 == rem) { // if n % 3 == 1
-            n += 4; // n % 3 == 2
-        }
+        n = nonMultipleOf3(n);

Review Comment:
   I think this is refactoring for the sake of refactoring. The code comments explain what is occurring. The comments here also match the comments for the `+2, +4` adjustment that happens a few lines below within the loop.



##########
commons-numbers-primes/src/main/java/org/apache/commons/numbers/primes/Primes.java:
##########
@@ -58,20 +58,35 @@ public static boolean isPrime(int n) {
         }
         return SmallPrimes.millerRabinPrimeTest(n);
     }
-
+    /**
+     * Return the number which is not a multiple of 3.
+     *
+     * @param n Positive number.
+     * @return number which is not a multiple of 3.
+     * @throws IllegalArgumentException if n < 0.
+     */
+    public static int nonMultipleOf3(int n) {
+        final int remainder = n % 3;
+        if (0 == remainder) { // if n % 3 == 0
+            n += 2; // n % 3 == 2
+        } else if (1 == remainder) { // if n % 3 == 1
+            n += 4; // n % 3 == 2
+        }
+        return n;
+    }
     /**
      * Return the smallest prime greater than or equal to n.
      *
      * @param n Positive number.
      * @return the smallest prime greater than or equal to {@code n}.
-     * @throws IllegalArgumentException if n < 0.
      */
     public static int nextPrime(int n) {
         if (n < 0) {
             throw new IllegalArgumentException(MessageFormat.format(NUMBER_TOO_SMALL, n, 0));
         }
         if (n <= 2) {
-            return 2;
+            int firstPrime = 2;

Review Comment:
   Creating a variable does not add anything to the code. This could all be done using a comment. Note that numbers allows the `magic` numbers beyond the usual set of `{-1, 0, 1}` as the entire library is supporting numerical operations. To create constants for well used constants such as 2 is unnecessary. Note also that loading the numbers -1 to 5 are single [byte code instructions](https://en.wikipedia.org/wiki/List_of_Java_bytecode_instructions). Since direct usage is more efficient, if any of these `int` constants is required then constants should only be used when comments cannot sufficiently clarify their usage.



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org