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

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 2

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.