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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
