Refactoring changes #129
Refactoring changes #129
Conversation
… Variable, Push down variable: Done
There was a problem hiding this comment.
I do not think these changes are an improvement. Please see individual comments for explanation.
Note that the ASF runs a SonarCloud instance to perform code quality checks. Many code smells identified by the analysis of numbers have been ignored. Reasons for doing so are recorded in the analysis reports. See SonarCloud Commons Numbers. The outstanding code smells are related to code complexity. Some algorithms possibly could use refactoring, or code comments to improve their maintainability. See Numbers Code Smells.
| Should be num uniform | ||
| Declared in ErfPerformance class. **/ | ||
| @Param({NUM_UNIFORM}) | ||
| private String NUM_TYPE; |
There was a problem hiding this 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 now 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.
| */ | ||
| 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; |
There was a problem hiding this 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. 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.
| } else if (1 == rem) { // if n % 3 == 1 | ||
| n += 4; // n % 3 == 2 | ||
| } | ||
| n = nonMultipleOf3(n); |
There was a problem hiding this 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.
Greetings team,
Thank you in advance for looking into this pull request.
I have refactored some changes to reduce some code smells
The changes are as follows
1)Angles.java
Pushed down variable: The variable TWO_PI was being used by Turn and Rad child classes of Angle parent class but Deg child
class was not using it so I pushed down the variable to both child classes, I also searched for all references to it and changed
accordingly.
2)ErfPerformance.java
Pulled up variable: Non uniform and Non uniform inverse string was being used by all sub classes so I pulled it up to the
parent class FunctionData
3)Primes.java
Renamed variable: changed the variable name from rem to remainder for better understanding
Added Explaining variable: There was a magic number 2 which was being returned I added an explaining variable called
FIRST_PRIME
Decomposed conditional: There was a conditional logic which was making the number n not a multiple of 3, I made it a
separate function for better understanding
None of the changes are causing any check style violation and maven build and maven test are also resulting in build success.
Thank you for this opportunity.