Extract Method with continue

Question

We're refactoring a long method; it contains a long for loop with many continue statements. I'd like to just use the Extract Method refactoring, but Eclipse's automated one doesn't know how to handle the conditional branching. I don't, either.

Our current strategy is to introduce a keepGoing flag (an instance variable since we're going to want to extract method), set it to false at the top of the loop, and replace every continue with setting the flag to true, then wrapping all the following stuff (at different nesting levels) inside an if (keepGoing) clause. Then perform the various extractions, then replace the keepGoing assignments with early returns from the extracted methods, then get rid of the flag.

Is there a better way?

Update: In response to comments - I can't share the code, but here's an anonymized excerpt:

private static void foo(C1 a, C2 b, C3 c, List<C2> list, boolean flag1) throws Exception {
 for (int i = 0; i < 1; i++) {
  C4 d = null;
  Integer e = null;
  boolean flag2 = false;
  boolean flag3 = findFlag3(a, c);
  blahblahblah();
  if (e == null) {
   if (flag1) {
    if (test1(c)) {
     if (test2(a, c)) {
      Integer f = getF1(b, c);
      if (f != null)
       e = getE1(a, f);
      if (e == null) {
       if (d == null) {
        list.add(b);
        continue;
       }
       e = findE(d);
      }
     } else {
      Integer f = getF2(b, c);
      if (f != null)
       e = getE2(a, f);
      if (e == null) {
       if (d == null) {
        list.add(b);
        continue;
       }
       e = findE(d);
      }
      flag2 = true;
     }
    } else {
     if (test3(a, c)) {
      Integer f = getF2(b, c);
      if (f != null)
       e = getE2(a, f);
      if (e == null) {
       if (d == null) {
        list.add(b);
        continue;
       }
       e = findE(d);
      }
      flag2 = true;
     } else {
      if (d == null) {
       list.add(b);
       continue;
      }
      e = findE(d);
      flag2 = true;
     }
    }
   }
   if (!flag1) {
    if (d == null) {
     list.add(b);
     continue;
    }
    e = findE(d);
   }
  }
  if (e == null) {
   list.add(b);
   continue;
  }
  List<C2> list2 = blahblahblah(b, list, flag1);
  if (list2.size() != 0 && flag1) {
   blahblahblah();
   if (!otherTest()) {
    if (yetAnotherTest()) {
     list.add(b);
     continue;
    }
    blahblahblah();
   }
  }
 }
}

Answer 1

continue is basically an analogue of an early return, right?

for (...) {
    doSomething(...);
}

private void doSomething(...) {
    ...
    if (...)
        return; // was "continue;"
    ...
    if (!doSomethingElse(...))
        return;
    ...
}

private boolean doSomethingElse(...) {
    ...
    if (...)
        return false; // was a continue from a nested operation
    ...
    return true;
}

Now I must admit that I didn't quite follow your current strategy, so I might have just repeated what you said. If so, then my answer is that I can't think of a better way.

Answer 2

If I were faced with your situation I would look at using other refactoring techniques such as "replace conditional with polymorphism". That said you should always do one thing at a time, so if you first want to extract method you have two options:

1. Add the "keepGoing" flag
2. Throw an exception from the method

Of these two options, I think the keepGoing flag is better. I wouldn't stop refactoring after you extract the method. I am sure once you have a smaller method you will find a way to remove this flag and have cleaner logic.

Answer 3

This is one of those fun ones where no single pattern will get you there.

I would work at it iteratively.

First I'd try to see if I couldn't use an early continue to remove one of those levels of ifs. It's much clearer code to check for a condition and return early (or in your case continue) than to have deeply nested ifs.

Next I think I'd take some of the inner chunks and see if they couldn't be extracted into a separate method. It looks like the first two big blocks (within the "if (test2(a, c)) {" and its else statement) are very similar. There is cut and paste logic that should be the same.

Finally after that stuff is cleared up, you can start looking at your actual problem--you need more classes. This entire statement is probably a three line polymorphic method in 3-5 sibling classes.

It's very close to throw-away and rewrite code, once you identify your actual classes, this entire method will vanish and be replaced with something so simple it hurts. Just the fact that it's a static utility method should be telling you something--you don't want one of those in this type of code.

Edit (After looking a little more): There is so much here it would be really fun to go through. Remember that when you are done you want no code duplication--and I'm pretty sure this entire thing could be written without a single if--I think all your ifs are cases that could/should easily be handled by polymorphism.

Oh, and as an answer to your question of eclipse not wanting to do it--don't even TRY automatic refactoring with this one, just do it by hand. The stuff inside that first if() needs to be pulled out into a method because it's virtually identical to the clause in its else()!

When I do something like this, I usually create a new method, move the code from the if into the new method (leaving just a call to the new method inside the if), then run a test and make sure you didn't break anything.

then go line by line and check to ensure there is no difference between the if and its else code. If there is, compensate for it by passing the difference as a new variable to the method. After you're sure everything is identical, replace the else clause with a call. Test again. Chances are at this point a few additional optimizations will become obvious, you'll most likely lose the entire if by combining it's logic with the variable you passed to differentiate the two calls.

Just keep doing stuff like that and iterating. The trick with refactoring is to use Very Small Steps and test between each step to ensure nothing changed.

Answer 4

As mmyers said, You can use return statements instead of continue in the extracted methods.

But eclipse will not allow you to extract the method with the continue statements in it, so either you can change all the continue statements to return and then extract the method or comment the continue statement and then extract the method, after extracting replace the continue with return statements.

I've followed First approach in one of my recent projects where I had to refactor spme of the code.

Answer 5

I'm going to summarize the answers here, while accepting Bill K's answer as the most complete. But everyone had something good to offer, and I might use any of these approaches next time I'm faced with this sort of situation.

---

mmyers: Cut out the loop body, paste it into a new method and replace all the continues with returns. This worked very nicely, although it would have trouble if there were other control flow statements, like break and return, inside the loop.

---

Bill K: Tease it apart iteratively; look for duplication and eliminate it. Take advantage of polymorphic classes to replace the conditional behavior. Use Very Small Steps. Yes; this is all good advice, with broader applicability than just this specific case.

---

Aaron: Either use the keepGoing flag to replace the continue or throw an Exception. I didn't try this, but I think the Exception option is a very nice alternative, and one I hadn't considered.