Refactoring a method for estimation

Question

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)) {
 double bestTaste = taste;
 //Try each star as a candidate bad star
 for (int badCand = 0; badCand < numStars; badCand++) {
 //Exclude candidate bad star
 int loop = 0;
 for (int star = 0; star < numStars; star++) {
 if (star != badCand) {
 starsBrfTry.set(loop, starsBrf.get(star));
 starsMeqTry.set(loop, starsMeq.get(star));
 loop = loop + 1;
 }
 }
 //Estimate attitude without candidate bad star
 QMethodValues estimatesTry = QMethod.compute(numStars - 1, starsBrfTry, starsMeqTry, measerrRef);
 double tasteTry = estimatesTry.getTaste();
 //Calculate probability of getting such a large value of TASTE at random
 double gammaPTry = new GammaP(0.5 * (2 * numStars - 5)).calc(0.5 * tasteTry);
 double probTry = 1.0 - gammaPTry;
 //If no error and we have found an acceptable value of taste that is better than
 //the best so far 
 if ((estimatesTry.isValid()) && (probTry >= probThresh) && (tasteTry < bestTaste)) {
 //Make a note of the details
 numBad = 1;
 idBad.set(0, ((Long ) starsIdsForRange.get(badCand)).intValue());
 /*quat = estimatesTry.getQuat();
 loss = estimatesTry.getLoss();
 taste = tasteTry;*/
 estimates = estimatesTry;
 prob = probTry;
 }
 }
 }
 //If still no error and a bad fit
 if ((estimates.isValid()) && (prob < probThresh)) {
 //Initialize best value of taste
 double bestTaste = taste;
 //Try each pair of stars as a candidate bad star pair 
 //bad_cand_1 \in {0, 1,..., num_stars - 2}
 for (int badCand_1 = 0; badCand_1 < numStars - 1; badCand_1++) {
 //bad_cand_2 \in {bad_cand_1 + 1, bad_cand_1 + 2,..., num_stars - 1}
 for (int loop_2 = 0; loop_2 < numStars - 1 - badCand_1; loop_2++) {
 int bad_cand_2 = loop_2 + badCand_1 + 1;
 //Exclude candidate bad star pair
 int loop = 0;
 for (int star = 0; star < numStars; star++) {
 if ((star != badCand_1) && (star != bad_cand_2)) {
 starsBrfTry.set(loop, starsBrf.get(star));
 starsMeqTry.set(loop, starsMeq.get(star));
 loop = loop + 1;
 }
 }
 //Estimate attitude without candidate bad star
 QMethodValues estimatesTry = QMethod.compute(numStars - 2, starsBrfTry, starsMeqTry, measerrRef);
 double tasteTry = estimatesTry.getTaste();
 //Calculate probability of getting such a large value of TASTE at random
 double gammaPTry = new GammaP(0.5 * (2 * numStars - 7)).calc(0.5 * tasteTry);
 double probTry = 1.0 - gammaPTry;
 //If no error and we have found an acceptable value of taste that is better than
 //the best so far 
 if ((estimatesTry.isValid()) && (probTry >= 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);
}

• RubberDuck: Hi there! Interesting question, but could you please tell us more about what the code does? Better titles and explaining your code in plain English makes it more likely that you'll receive an answer.
• IEatBagels: Is your program estimating Justin Bieber's attitude? :p Or... a sun-like star

Answer 1

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:

• 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.
• loop isn'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.

Answer 2

I was originally trying to work through the method as a whole and refactor it, but I simply don't have enough information about what's going on. In addition to what @tim is saying, I'd recommend:

Executing within if

I'd personally flip the if statements that are doing the checking whether the program can run, so your

if ((estimates.isValid()) && (prob < probThresh))

and make it

if (!estimates.isValid() || prob >= probThresh)

then throw an exception, or signal an error somehow, and exit there, as opposed to having your entire body inside an if statement.

Magic numbers

You have quite a lot of instances where you're adding or subtracting by constants, for example:

double gammaPTry = new GammaP(0.5 * (2 * numStars - 5)).calc(0.5 * tasteTry);

Reading that, I have no idea why you're subtracting 5 and multiplying by 2. Either a comment or some more descriptive code, either a constant variable or even a helper function would help with this.

Extracting duplicate code

This is the tricky bit. Your loops are similar but not identical. Without knowing exactly what they're doing it's difficult to say exactly what you should do, but putting them in another method that accepts the differences as arguments might be a starting point.

Other

Things like putting loop++ in place of loop = loop + 1, or even loop += 1, if the increment might change will increase readability, and deleting all of your commented code would help a lot.

• tim: you could simplify your proposed if statement by removing the extra parentheses around isValid and changing !(prob < probThresh) to prob >= probThresh

Answer 3

The first step I'd recommend is move nested code blocks (e.g. statements between a {} pair) into their own methods. This is a common refactoring that makes it easier to perform more specific reafactorings. I recommend you perform this and then update your question with the refactored code. From there more refactorings can be recommended.

Here is an example on a simple contrived example. Here is the class Foo that we'll refactor.

public class Foo {
 public void foo(String message, int count, int step) {
 for (int i = 0; i < count; i++) {
 if (i % step == 0) {
 message = message + " " + i;
 System.out.println(message);
 }
 }
 }
}

Here's some driver code.

public class Main {
 public static void main(String[] args) {
 new Foo().foo("Hello!", 10, 2);
 }
}

...and the output.

Hello! 0
Hello! 0 2
Hello! 0 2 4
Hello! 0 2 4 6
Hello! 0 2 4 6 8

1) Move the if's code block into its own method. Looking at the code block we see it uses the variables message and i, these will be are method's parameters. Also, it reassigns to message, so message will be our return.

public class Foo {
 public void foo(String message, int count, int step) {
 for (int i = 0; i < count; i++) {
 if (i % step == 0) {
 message = updateMessage(message, i);
 }
 }
 }
 private String updateMessage(String message, int i) {
 message = message + " " + i;
 System.out.println(message);
 return message;
 }
}

2) Do it again for the for loop code block.

public class Foo {
 public void foo(String message, int count, int step) {
 for (int i = 0; i < count; i++) {
 message = updateMessageIfStep(message, i, step);
 }
 }
 private String updateMessageIfStep(String message, int i, int step) {
 if (i % step == 0) {
 return updateMessage(message, i);
 } else {
 return message;
 }
 }
 private String updateMessage(String message, int i) {
 message = message + " " + i;
 System.out.println(message);
 return message;
 }
}