2010-11-14 4 views
7

Ich habe eine Frage bezüglich Code-Duplikation und Refactoring, hoffe, es ist nicht zu allgemein. Angenommen, Sie haben ein ziemlich kleines Stück Code (~ 5 Zeilen), das ist eine Folge von Funktionsaufrufen das ist - nicht ein sehr niedriges Niveau. Dieser Code wird an mehreren Stellen wiederholt, daher wäre es wahrscheinlich eine gute Idee, hier eine Methode zu extrahieren. In diesem speziellen Beispiel würde diese neue Funktion jedoch unter einer geringen Kohäsion leiden (was sich unter anderem darin äußert, dass es schwierig ist, einen guten Namen für die Funktion zu finden). Der Grund dafür ist wahrscheinlich, dass dieser wiederholte Code nur ein Teil eines größeren Algorithmus ist - und es ist schwierig, ihn in gut benannte Schritte zu unterteilen.Um zu trocknen oder nicht zu trocknen? Auf Vermeidung von Code-Duplizierung und Zusammenhalt erhalten

Was würden Sie in einem solchen Szenario vorschlagen?

Edit:

Ich wollte die Frage auf einer allgemeinen Ebene zu halten, so dass mehr Menschen potenziell es nützlich finden, aber natürlich wäre es am besten, es mit einiger Code Probe zu sichern. Das Beispiel ist vielleicht nicht die beste sein, je (es riecht in ganz wenigen Möglichkeiten), aber ich hoffe, es macht seinen Job:

class SocketAction { 

    private static class AlwaysCreateSessionLoginHandler extends LoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID()); 
      socketAction.registerSession(); 
      socketAction._sess.runApplication(); 
     } 
    } 

    private static class AutoConnectAnyDeviceLoginHandler extends LoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      if (Server.isUserRegistered(socketAction._sess.getUserLogin())) { 
       Log.logSysInfo("Session autoconnect - acquiring list of action threads..."); 
       String[] sa = Server.getSessionList(socketAction._sess.getUserID()); 
       Log.logSysInfo("Session autoconnect - list of action threads acquired."); 
       for (int i = 0; i < sa.length; i += 7) { 
        socketAction.abandonCommThreads(); 
        Server.attachSocketToSession(sa[i + 1], socketAction._commSendThread.getSock()); 
        return; 
       } 
      } 
      Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID()); 
      socketAction.registerSession(); 
      socketAction._sess.runApplication(); 
     } 
    } 

    private static class OnlyNewSessionLoginHandler extends LoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      socketAction.killOldSessionsForUser(); 
      Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID()); 
      socketAction.registerSession(); 
      socketAction._sess.runApplication(); 
     } 
    } 
} 

Antwort

8

Frage ist zu allgemein wirklich zu sagen, aber als Übung:

Angenommen, Sie abstrahieren es. Denken Sie darüber nach, was die wahrscheinlichen Gründe dafür sind, die resultierende 5-Linien-Funktion zu ändern. Möchten Sie wahrscheinlich Änderungen vornehmen, die für alle Benutzer gelten, oder würden Sie am Ende eine neue Funktion schreiben müssen, die sich leicht von der alten unterscheidet und jedes Mal, wenn ein Anrufer eine Änderung wünscht?

Wenn Sie es für alle Benutzer ändern möchten, ist es eine realisierbare Abstraktion. Geben Sie ihm jetzt einen schlechten Namen, vielleicht denken Sie später an einen besseren.

Wenn Sie diese Funktion in viele ähnliche Versionen aufteilen, wie sich Ihr Code in Zukunft entwickelt, ist dies wahrscheinlich keine brauchbare Abstraktion. Sie könnten die Funktion zwar schreiben, aber es handelt sich eher um eine Code-speichernde "Hilfsfunktion" als um einen Teil Ihres formalen Modells des Problems. Dies ist nicht sehr befriedigend: die Wiederholung dieser Menge an Code ist ein wenig besorgniserregend, weil es darauf hindeutet, sollte eine brauchbare Abstraktion dort irgendwo sein.

Vielleicht könnten 4 der 5 Zeilen abstrahiert werden, da sie zusammenhängender sind, und die fünfte Zeile passiert gerade so mit ihnen im Moment. Dann könnten Sie 2 neue Funktionen schreiben: eine, die diese neue Abstraktion ist, und die andere ist nur ein Helfer, der die neue Funktion aufruft und dann Zeile 5 ausführt. Eine dieser Funktionen könnte dann eine längere erwartete Nutzungsdauer haben als die andere.

+0

Ich versuche wirklich, ein kompaktes Stück Code zu finden, um meine Frage zu unterstützen, aber die Code-Basis, die ich mich selbst herausgefordert habe, zu refaktorieren, scheint so durcheinander zu sein, dass es schwierig ist, das Problem zu isolieren Es wäre wahrscheinlich schwierig, etwas einzufügen, ohne in eine lange und langweilige Beschreibung zu gehen. Ich denke, Sie haben es mit dem Vorschlag "Es sollte eine brauchbare Abstraktion da irgendwo geben" genagelt. Es ist so ein starkes Gefühl, dass ich immer noch den Code anstarre und mich weigere, loszulassen. Weil ich einfach weiß * gibt es einen besseren Weg, die ganze Idee auszudrücken als dieses Spaghetti-Zeug! – lukem00

+0

Ich habe einige Codebeispiele hinzugefügt - möchten Sie die Beschreibung des allgemeinen Ansatzes mit einigen spezifischen Tipps und Vorschlägen ergänzen - zur besseren Veranschaulichung? – lukem00

+0

@ lukem00: Ich weiß nicht, vielleicht könnten diese 3 gemeinsamen Linien in eine Methode von 'SocketAction' umgewandelt werden, auch 'onLoginCorrect' genannt? Das heißt, jeder 'LoginHandler' soll sein Ding machen und dann an die' SocketAction' delegieren, die diese Session-Sachen erledigt. –

1

Ich habe in letzter Zeit darüber nachgedacht und ich verstehe genau, worauf Sie hinaus wollen. Es fällt mir ein, dass dies eine Implementierungsabstraktion mehr als alles andere ist und daher schmackhafter ist, wenn Sie vermeiden können, eine Schnittstelle zu ändern. Zum Beispiel könnte ich in C++ die Funktion in die cpp extrahieren, ohne den Header zu berühren. Dies verringert etwas die Anforderung, dass die Funktionsabstraktion für den Klassenbenutzer wohlgeformt und bedeutungsvoll ist, da sie für sie unsichtbar ist, bis sie sie wirklich benötigt (wenn sie zu der Implementierung hinzugefügt wird).

+0

Ja, das sind interne Sachen, also bleibt die Schnittstelle gleich. Vielleicht ist das eine utopische Denkweise, aber ich glaube immer noch, dass auch private Methoden anständige Abstraktionen bilden sollten. Code Duplikation ist ein Problem und Lesbarkeit ist ein anderes.Ich wünschte, alle meine Methoden würden wie Becks * Composed Method * aussehen und so gut lesbar sein wie Cunninghams "November (20, 2005)" ... :) – lukem00

1

Für mich ist das operative Wort "Schwelle". Ein anderes Wort dafür wäre wahrscheinlich "Geruch".

Die Dinge sind immer in einer Balance. Es klingt wie (in diesem Fall) wie das Zentrum der Balance in Kohäsion (cool) ist; und da du nur eine kleine Anzahl von Duplikaten hast, ist es nicht schwer zu verwalten.

Wenn Sie einige wichtige "Ereignis" auftreten und Sie gingen zu "1000" Duplikate dann hätte das Gleichgewicht verschoben und so könnten Sie nähern.

Für mich sind einige managable Duplikate (noch) kein Signal zum Refactor; aber ich behalte es im Auge.

+0

OK, es ist * peinlich, dass dieses Thema in zwei Fragen aufgeteilt wird - Entschuldigung über das. Ich denke, die ganze Frage nach dem Code, den ich zeige, sollte eigentlich so etwas wie "Wie würde man das Ganze umgestalten, um es lesbarer zu machen?" - Denn das ist das eigentliche Problem, das ich hier habe. Ich befürchte, dass niemand es lesen würde, also habe ich versucht, es neu zu formulieren, so dass es für mehr Leute als nur für mich relevant erscheint. – lukem00

0

Vererbung ist dein Freund!

Kopieren Sie den Code nicht. Auch wenn eine einzelne Codezeile sehr lang oder schwierig ist, refaktorieren Sie sie in eine separate Methode mit einem eindeutigen Namen. Stellen Sie es sich vor wie jemand, der Ihren Code in einem Jahr lesen wird. Wenn Sie diese Funktion "blabla" nennen, wird der nächste Typ wissen, was diese Funktion macht, ohne ihren Code zu lesen? Wenn nicht, müssen Sie den Namen ändern. Nach einer Woche des Denkens werden Sie sich daran gewöhnen und Ihr Code wird 12% besser lesbar sein! ;)

class SocketAction { 

    private static abstract class CreateSessionLoginHandler extends LoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID()); 
      socketAction.registerSession(); 
      socketAction._sess.runApplication(); 
     } 
    } 

    private static class AlwaysCreateSessionLoginHandler extends CreateSessionLoginHandler; 

    private static class AutoConnectAnyDeviceLoginHandler extends CreateSessionLoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      if (Server.isUserRegistered(socketAction._sess.getUserLogin())) { 
       Log.logSysInfo("Session autoconnect - acquiring list of action threads..."); 
       String[] sa = Server.getSessionList(socketAction._sess.getUserID()); 
       Log.logSysInfo("Session autoconnect - list of action threads acquired."); 
       for (int i = 0; i < sa.length; i += 7) { 
        socketAction.abandonCommThreads(); 
        Server.attachSocketToSession(sa[i + 1], socketAction._commSendThread.getSock()); 
        return; 
       } 
      } 
      super.onLoginCorrect(socketAction); 
     } 
    } 

    private static class OnlyNewSessionLoginHandler extends CreateSessionLoginHandler { 
     @Override 
     protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException { 
      socketAction.killOldSessionsForUser(); 
      super.onLoginCorrect(socketAction); 
     } 
    } 
} 
2

Für mich ist der Lackmus-Test: Wenn ich an einem Ort eine Änderung an dieser Folge von Code vornehmen müssen, (zB eine Zeile hinzufügen, die Reihenfolge) ändern, müssen würde ich machen die gleiche Änderung an anderen Standorten?

Wenn die Antwort ja ist, dann ist es eine logische, "atomare" Entität und sollte refaktorisiert werden. Wenn die Antwort nein lautet, handelt es sich um eine Abfolge von Vorgängen, die in mehr als einer Situation funktionieren - und wenn sie umstrukturiert werden, werden Sie wahrscheinlich in Zukunft mehr Probleme bekommen.

Verwandte Themen