patternjavaMinor
Refactoring a method for estimation
Viewed 0 times
refactoringestimationformethod
Problem
I have to refactor this method: it seems to duplicate two code blocks with some slight variation. Any suggestions? It is a bit of a mess :)
public EstimatedAttitude estimate(int numStars, Double2d starsBrf, Double2d starsMeq, Long1d starsIdsForRange,
double measerrRef, double probThresh) {
Double2d starsBrfTry = new Double2d(9, 3);
Double2d starsMeqTry = new Double2d(9, 3);
Int1d idBad = new Int1d(2);
int numBad = 0;
//Estimate the attitude using q-method
QMethodValues estimates = QMethod.compute(numStars, starsBrf, starsMeq, measerrRef);
//double loss = estimates.getLoss();
//Quaternion quat = estimates.getQuat();
//Calculate probability of getting such a large value of TASTE at random
double taste = estimates.getTaste();
double gammaP = new GammaP(0.5 * (2 * numStars - 3)).calc(0.5 * taste);
double prob = 1.0 - gammaP;
//If no error and a bad fit
if ((estimates.isValid()) && (prob = probThresh) && (tasteTry = probThresh) && (tasteTry < bestTaste)) {
//Make a note of the details
numBad = 2;
idBad.set(0, ((Long ) starsIdsForRange.get(badCand_1)).intValue());
idBad.set(1, ((Long ) starsIdsForRange.get(bad_cand_2)).intValue());
/*quat = estimatesTry.getQuat();
loss = estimatesTry.getLoss();
taste = tasteTry;*/
estimates = estimatesTry;
prob = probTry;
}
}
}
}
//create a new qmethod value with correct fields
return new EstimatedAttitude(estimates, prob, numBad, idBad);
}Solution
Unit Tests
This is probably going to be a major rewrite, so the first thing I would do is write unit tests for it. That way, you can confirm that your new version actually will do the same thing as the original version.
Naming
The current names don't really tell a reader all that much:
You should also change the variables so that they comply with Java naming standards (camelCase instead of snake_case).
Remove commented out code
Remove all commented out code, as it makes the code harder to read. If you think that it might be needed in the future, use version control.
Comments
You need more comments in your code. At least you would need a JavaDoc for
Refactoring
Without working code, unit tests, and a general description of what the code actually does, it will be really hard for us to refactor this.
My general approach would be to identify similar code at a small scope, extract both versions of it to new methods (with appropriate arguments and return values), and then see if I can change the signature and workings of those methods to use the same method for both cases. If it does work (confirm correctness with unit tests), do the same thing for the next biggest scope.
During this process, I would also keep an eye out if creating objects might simplify some of the code.
This is probably going to be a major rewrite, so the first thing I would do is write unit tests for it. That way, you can confirm that your new version actually will do the same thing as the original version.
Naming
The current names don't really tell a reader all that much:
starsBrf&starsMeq: what is a brf star? and what is a meq? if they are technical terms, describe them in a (JavaDoc) comment, if not, use better names.
loopisn't really a loop, it's a counter. but what does it count?
idBad&numBad: what is a bad? and what does the num do for it?
estimates: estimates of what?
- what is a taste, and what is a try?
- and so on.
You should also change the variables so that they comply with Java naming standards (camelCase instead of snake_case).
Remove commented out code
Remove all commented out code, as it makes the code harder to read. If you think that it might be needed in the future, use version control.
Comments
You need more comments in your code. At least you would need a JavaDoc for
estimate. What does it estimate? What parameters does it except? How does it estimate?Refactoring
Without working code, unit tests, and a general description of what the code actually does, it will be really hard for us to refactor this.
My general approach would be to identify similar code at a small scope, extract both versions of it to new methods (with appropriate arguments and return values), and then see if I can change the signature and workings of those methods to use the same method for both cases. If it does work (confirm correctness with unit tests), do the same thing for the next biggest scope.
During this process, I would also keep an eye out if creating objects might simplify some of the code.
Context
StackExchange Code Review Q#67785, answer score: 9
Revisions (0)
No revisions yet.