Extraktmethode mit Fortsetzung

Question

Wir refaktorisieren eine lange Methode; sie enthält eine lange for Schleife mit vielen continue Erklärungen. Ich würde gerne einfach die Auszug Methode Refactoring, aber das automatische Refactoring von Eclipse weiß nicht, wie es mit der bedingten Verzweigung umgehen soll. Ich weiß es auch nicht.

Unsere derzeitige Strategie ist die Einführung eines keepGoing Flagge (eine Instanzvariable, da wir in der Zukunft Extraktmethode ), setzen Sie es am Anfang der Schleife auf false, und ersetzen Sie jedes continue durch das Setzen des Flags auf true, um dann alles Folgende (auf verschiedenen Verschachtelungsebenen) in eine if (keepGoing) Klausel. Führen Sie dann die verschiedenen Extraktionen durch und ersetzen Sie dann die keepGoing Zuweisungen mit frühzeitigen Erträgen aus den extrahierten Methoden, dann werden Sie das Kennzeichen los.

Gibt es einen besseren Weg?

Update : Als Antwort auf Kommentare - ich kann den Code nicht weitergeben, aber hier ist ein anonymisierter Auszug:

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

Dies ist eine dieser lustigen Aufgaben, bei denen kein einziges Muster ausreicht, um das Ziel zu erreichen.

Ich würde iterativ vorgehen.

Zuerst würde ich versuchen, ob ich nicht ein frühes continue verwenden kann, um eine dieser Ebenen von ifs zu entfernen. Es ist viel klarer Code für eine Bedingung zu überprüfen und früh zurückkehren (oder in Ihrem Fall fortsetzen) als tief verschachtelte ifs haben.

Als Nächstes würde ich einige der inneren Teile nehmen und sehen, ob sie nicht in eine separate Methode extrahiert werden können. Es sieht so aus, als ob die ersten beiden großen Blöcke (innerhalb der "if (test2(a, c)) {" und ihrer else-Anweisung) sehr ähnlich sind. Es gibt eine Ausschneide- und Einfügelogik, die die gleiche sein sollte.

Wenn das geklärt ist, können Sie sich endlich um Ihr eigentliches Problem kümmern - Sie brauchen mehr Klassen. Diese gesamte Anweisung ist wahrscheinlich eine dreizeilige polymorphe Methode in 3-5 Geschwisterklassen.

Sobald Sie Ihre eigentlichen Klassen identifiziert haben, wird diese gesamte Methode verschwinden und durch etwas so Einfaches ersetzt werden, dass es weh tut. Allein die Tatsache, dass es sich um eine statische Utility-Methode handelt, sollte Ihnen etwas sagen - so etwas wollen Sie in dieser Art von Code nicht.

Bearbeiten (nachdem ich ein wenig mehr gesucht habe): Es gibt hier so viel, dass es wirklich Spaß machen würde, es durchzugehen. Denken Sie daran, dass, wenn Sie fertig sind, Sie keine Code-Duplizierung wollen - und ich bin ziemlich sicher, dass diese ganze Sache ohne ein einziges if geschrieben werden könnte - ich denke, alle Ihre ifs sind Fälle, die leicht durch Polymorphismus behandelt werden könnten/sollten.

Oh, und als Antwort auf Ihre Frage, ob Eclipse es nicht tun will - versuchen Sie es gar nicht erst mit dem automatischen Refactoring, machen Sie es einfach von Hand. Das Zeug in der ersten if() muss in eine Methode ausgelagert werden, weil es praktisch identisch mit der Klausel in ihrer else() ist!

Wenn ich so etwas tue, erstelle ich in der Regel eine neue Methode, verschiebe den Code aus dem if in die neue Methode (so dass nur ein Aufruf der neuen Methode innerhalb des if), dann führen Sie einen Test und stellen Sie sicher, dass Sie nichts gebrochen haben.

gehen Sie dann Zeile für Zeile durch und prüfen Sie, ob es keinen Unterschied zwischen dem if- und dem else-Code gibt. Falls doch, gleichen Sie dies aus, indem Sie die Differenz als neue Variable an die Methode übergeben. Wenn Sie sicher sind, dass alles identisch ist, ersetzen Sie die else-Klausel durch einen Aufruf. Testen Sie erneut. Wahrscheinlich werden an dieser Stelle ein paar zusätzliche Optimierungen deutlich, denn Sie werden höchstwahrscheinlich den gesamten if-Code verlieren, indem Sie seine Logik mit der Variable kombinieren, die Sie zur Unterscheidung der beiden Aufrufe übergeben haben.

Machen Sie einfach weiter mit solchen Dingen und iterieren Sie. Der Trick beim Refactoring ist, in sehr kleinen Schritten zu arbeiten und zwischen den einzelnen Schritten zu testen, um sicherzustellen, dass sich nichts geändert hat.

Answer 2

continue ist im Grunde eine Analogie zu einer vorzeitigen Rückkehr, oder?

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;
}

Nun muss ich zugeben, dass ich Ihre derzeitige Strategie nicht ganz nachvollziehen konnte, so dass ich vielleicht nur wiederholt habe, was Sie gesagt haben. Wenn das so ist, dann ist meine Antwort, dass ich mir keinen besseren Weg vorstellen kann.

Answer 3

Wenn ich mit Ihrer Situation konfrontiert wäre, würde ich andere Refactoring-Techniken wie "Ersetze Bedingung durch Polymorphismus" in Betracht ziehen. Abgesehen davon sollten Sie immer nur eine Sache auf einmal tun. Wenn Sie also zuerst eine Methode extrahieren möchten, haben Sie zwei Möglichkeiten:

1. Das Flag "keepGoing" hinzufügen
2. Eine Ausnahme aus der Methode auslösen

Von diesen beiden Optionen halte ich das keepGoing-Flag für besser. Ich würde das Refactoring nicht beenden, nachdem Sie die Methode extrahiert haben. Ich bin sicher, sobald Sie eine kleinere Methode haben, werden Sie einen Weg finden, dieses Flag zu entfernen und eine sauberere Logik zu haben.

Answer 4

Ich werde die Antworten hier zusammenfassen, wobei ich die Antwort von Bill K. als die vollständigste akzeptiere. Aber jeder hatte etwas Gutes zu bieten, und ich könnte jeden dieser Ansätze verwenden, wenn ich das nächste Mal mit einer solchen Situation konfrontiert werde.

---

mmyers : Schneiden Sie den Schleifenkörper aus, fügen Sie ihn in eine neue Methode ein und ersetzen Sie alle continue s mit return s. Dies funktionierte sehr gut, obwohl es Probleme geben würde, wenn es andere Kontrollflussanweisungen wie break und return innerhalb der Schleife gäbe.

---

Rechnung K : Zerlegen Sie sie iterativ; suchen Sie nach Überschneidungen und beseitigen Sie diese. Nutzen Sie die Vorteile polymorpher Klassen, um das bedingte Verhalten zu ersetzen. Verwenden Sie sehr kleine Schritte. Ja, das sind alles gute Ratschläge, die nicht nur in diesem speziellen Fall anwendbar sind.

---

Aaron : Entweder verwenden Sie die keepGoing Flagge, um die continue oder eine Exception auslösen. Ich habe das nicht ausprobiert, aber ich denke, die Option "Ausnahme" ist eine sehr gute Alternative, die ich nicht in Betracht gezogen hatte.