2009-04-27 4 views
8

Ich bin gerade auf ein Muster gestoßen, das ich vorher gesehen habe, und wollte Meinungen dazu bekommen. Der betreffende Code beinhaltet eine Schnittstelle wie folgt aus:Secret Handshake Anti-Pattern

public interface MyCrazyAnalyzer { 
    public void setOptions(AnalyzerOptions options); 
    public void setText(String text); 
    public void initialize(); 
    public int getOccurances(String query); 
} 

Und die erwartete Nutzung ist wie folgt:

MyCrazyAnalyzer crazy = AnalyzerFactory.getAnalyzer(); 
crazy.setOptions(true); 
crazy.initialize(); 
Map<String, Integer> results = new HashMap<String, Integer>(); 
for(String item : items) { 
    crazy.setText(item); 
    results.put(item, crazy.getOccurances); 
} 

ist es Gründe für einen Teil davon. Die setText (...) und getOccurances (...) sind da, weil es mehrere Abfragen gibt, die Sie nach der gleichen teuren Analyse der Daten durchführen möchten, aber dies kann zu einer Ergebnisklasse refaktoriert werden.

Warum ich denke, das ist so schlecht: Die Implementierung speichert Zustand in einer Weise, die nicht eindeutig von der Schnittstelle angezeigt wird. Ich habe auch etwas Ähnliches gesehen, das eine Schnittstelle betrifft, die "prepareResult" und dann "getResult" aufrufen muss. Jetzt kann ich an gut entworfenen Code denken, der einige dieser Eigenschaften einsetzt. Die Hadoop-Mapper-Schnittstelle erweitert JobConfigurable und Closeable, aber ich sehe einen großen Unterschied, weil es ein Framework ist, das Benutzercode verwendet, der diese Schnittstellen implementiert, im Gegensatz zu einem Service, der mehrere Implementierungen haben kann. Ich nehme an, dass alles, was mit der Aufnahme einer "nahen" Methode zu tun hat, die aufgerufen werden muss, gerechtfertigt ist, da es keinen anderen vernünftigen Weg gibt, dies zu tun. In einigen Fällen, wie JDBC, ist dies eine Konsequenz einer undichten Abstraktion, aber in den beiden Code-Stücken, an die ich denke, ist es ziemlich klar, dass Programmierer hastig eine Schnittstelle zu einer Spaghetti-Code-Klasse hinzufügen, um sie zu bereinigen.

Meine Fragen sind:

  1. Hat jeder zustimmen, dass dieses eine schlecht gestaltete Oberfläche ist?
  2. Ist dies ein beschriebenes Anti-Pattern?
  3. Gehört diese Art von Initialisierung jemals in eine Schnittstelle?
  4. Scheint das nur falsch für mich, weil ich eine Präferenz für funktionellen Stil und Unveränderlichkeit habe?

Wenn dies häufig genug ist, einen Namen zu verdienen, schlage ich vor, das „Secret Handshake“ anti-Muster für eine Schnittstelle, die Sie zwingt mehrere Methoden in einer bestimmten Reihenfolge zu nennen, wenn die Schnittstelle nicht von Natur aus Stateful ist (wie eine Sammlung).

+0

Um ehrlich zu sein, erinnert mich dieses "Muster" an DataReaders in ADO.NET. Das bedeutet nicht, dass es notwendigerweise eine gute Idee ist, aber es wird sogar in ernsthaften APIs verwendet. –

+0

Gutes Anti-Muster. Wenn Objekte so initialisiert werden sollten, könnten wir Konstruktoren in Interfaces angeben. Ich mag auch den Begriff "geheimer Händedruck". – CurtainDog

+0

Zumindest ist es explizit schlecht, im Gegensatz zu java.util.MessageFormat, die temporären internen Zustand während einzelner Methodenaufrufe verwendet. –

Antwort

17

Ja, es ist ein Anti-Muster: Sequential coupling.

Ich würde in Options refaktorieren - an die Fabrik übergeben, und Results, zurückgegeben von einer analyseText() Methode.

+13

Ausgezeichnet, jetzt kann ich "Remove Sequential Coupling" in meine Checkin-Nachrichten statt die weniger höflich "verwirrten Müll entfernen". –

3
  1. Ich würde erwarten, dass die AnalyzerFactory die notwendigen Parameter erhalten und die Konstruktion selbst durchführen; sonst, was genau macht es?
  2. Nicht sicher, ob es einen Namen hat, aber es scheint wie es sollte :)
  3. Ja, gelegentlich ist es bequem (und die richtige Ebene der Abstraktion), Setter in Ihrer Schnittstelle zu haben und Klassen zu erwarten, sie zu nennen. Ich würde vorschlagen, dass dies erfordert umfangreiche Dokumentation dieser Tatsache.
  4. Nicht wirklich, nein. Eine Präferenz für Unveränderlichkeit ist sicherlich eine gute Sache, und Setter/Bean-basiertes Design kann manchmal die "richtige" Wahl sein, aber Ihr gegebenes Beispiel nimmt es zu weit.
+0

+1 Weil ich mich gefragt habe, was die Fabrik auch macht ... scheint es, dass es nur als globale Ressource benutzt wird, ein weiteres No-No. – CurtainDog

3

Ich bin mir nicht sicher, ob es ein beschriebenes Anti-Pattern ist, aber ich stimme völlig zu, dass dies eine schlecht gestaltete Schnittstelle ist. Es lässt zu viele Möglichkeiten für Fehler und verletzt mindestens ein Schlüsselprinzip: Machen Sie Ihre API schwer zu missbrauchen.

Neben Missbrauch kann diese API auch zu schwer zu debuggenden Fehlern führen, wenn mehrere Threads dieselbe Instanz verwenden.

Joshua Bloch hat tatsächlich eine ausgezeichnete presentation (36m16s und 40m30s) auf API-Design und er adressiert dies als eine der Eigenschaften einer schlecht gestalteten API.

0

Ich kann nichts Schlechtes hier sehen. setText() bereitet die Bühne vor; Danach haben Sie einen oder mehrere Anrufe zu getOccurances(). Da setText() so teuer ist, kann ich mir keinen anderen Weg vorstellen, dies zu tun.

getOccurances(text, query) würde den "geheimen Handshake" zu einem enormen Leistungsaufwand beheben. Sie könnten versuchen, Text in getOccurances() zwischenzuspeichern und nur Ihre internen Caches zu aktualisieren, wenn sich der Text ändert, aber das mehr und mehr nach einem OO-Prinzip zu suchen scheint. Wenn eine Regel keinen Sinn ergibt, wenden Sie sie nicht an. Softwareentwickler haben ein Gehirn aus einem Grund.

+0

Wie Douglas vorschlägt, ist es möglich, diese Art von Code in etwas umzuwandeln, das ein Result-Objekt zurückgibt. –

0

Eine mögliche Lösung - Fluent chaning verwenden. Das vermeidet eine Klasse, die Methoden enthält, die in einer bestimmten Reihenfolge aufgerufen werden müssen. Es ähnelt stark dem Builder-Muster, das sicherstellt, dass Sie keine Objekte lesen, die sich noch in der Mitte befinden.