Auf diese Weise schreibe ich diesen Code ist testbar, aber ist etwas falsch daran, was ich vermisse?

13

Ich habe eine Schnittstelle aufgerufen IContext. Zu diesem Zweck spielt es keine Rolle, was es tut, mit Ausnahme der folgenden:

T GetService<T>();

Diese Methode überprüft den aktuellen DI-Container der Anwendung und versucht, die Abhängigkeit aufzulösen. Ziemlich normal, denke ich.

In meiner ASP.NET MVC-Anwendung sieht mein Konstruktor folgendermaßen aus.

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetService<ISomeService>();
    AnotherService = ctx.GetService<IAnotherService>();
}

So anstatt für jeden Dienst mehr Parameter im Konstruktor hinzugefügt (weil das ist wirklich ärgerlich wird und zeitraubend für die Entwickler zur Verlängerung der Anwendung) ich diese Methode bin mit Dienstleistungen zu erhalten.

Jetzt fühlt es sich falsch an . Die Art und Weise, wie ich es derzeit in meinem Kopf rechtfertige, ist jedoch folgende: Ich kann mich darüber lustig machen .

Ich kann. Es wäre nicht schwierig, sich zu verspotten IContext, um den Controller zu testen. Ich müsste sowieso:

public class MyMockContext : IContext
{
    public T GetService<T>()
    {
        if (typeof(T) == typeof(ISomeService))
        {
            // return another mock, or concrete etc etc
        }

        // etc etc
    }
}

Aber wie gesagt, es fühlt sich falsch an. Irgendwelche Gedanken / Missbrauch willkommen.

LiverpoolsNumber9
quelle
8
Dies nennt man Service Locator und ich mag es nicht. Zu diesem Thema wurde viel geschrieben - siehe martinfowler.com/articles/injection.html und blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern für den Anfang .
Benjamin Hodgson
Aus dem Artikel von Martin Fowler: "Ich habe oft die Beschwerde gehört, dass solche Service-Locators eine schlechte Sache sind, weil sie nicht testbar sind, weil Sie sie nicht durch Implementierungen ersetzen können. Natürlich können Sie sie schlecht entwerfen, um darauf einzugehen In diesem Fall ist die Service Locator-Instanz nur ein einfacher Datenbehälter. Ich kann den Locator problemlos mit Testimplementierungen meiner Services erstellen. " Kannst du erklären, warum es dir nicht gefällt? Vielleicht in einer Antwort?
LiverpoolsNumber9
8
Er hat recht, das ist schlechtes Design. Es ist einfach: public SomeClass(Context c). Dieser Code ist ziemlich klar, nicht wahr? Es heißt, es that SomeClasskommt auf a an Context. Ähm, aber warte, das tut es nicht! Es hängt nur von der Abhängigkeit ab, die Xes vom Kontext erhält. Das heißt, macht jedes Mal , wenn eine Änderung Contextes könnte brechen SomeObject, auch wenn Sie nur geändert Contexts Y. Aber ja, du weißt, dass du dich nur Ynicht verändert hast X, also SomeClassist es in Ordnung. Beim Schreiben eines guten Codes geht es jedoch nicht darum, was Sie wissen, sondern darum , was der neue Mitarbeiter weiß, wenn er sich Ihren Code zum ersten Mal ansieht.
Valentinstag,
@ DocBrown Für mich ist das genau das, was ich gesagt habe - ich sehe hier keinen Unterschied. Können Sie das bitte näher erläutern?
Valentinstag
1
@ DocBrown Ich verstehe deinen Standpunkt jetzt. Ja, wenn sein Kontext nur ein Bündel aller Abhängigkeiten ist, dann ist dies kein schlechtes Design. Es mag jedoch eine schlechte Benennung sein, aber das ist auch nur eine Vermutung. OP sollte klären, ob es mehr Methoden (innere Objekte) im Kontext gibt. Auch die Diskussion von Code ist in Ordnung, aber das ist programmers.stackexchange, daher sollten wir für mich auch versuchen, "hinter" die Dinge zu schauen, die das OP verbessern.
Valentinstag

Antworten:

5

Ein statt vieler Parameter im Konstruktor zu haben, ist nicht der problematische Teil dieses Entwurfs . Solange Ihre IContextKlasse nichts anderes als eine Servicefassade ist , insbesondere zur Bereitstellung der in verwendeten Abhängigkeiten MyControllerBase, und kein allgemeiner Service-Locator, der im gesamten Code verwendet wird, ist dieser Teil Ihres Codes meiner Meinung nach in Ordnung.

Ihr erstes Beispiel könnte in geändert werden

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetSomeService();
    AnotherService = ctx.GetAnotherService();
}

das wäre keine wesentliche konstruktionsänderung von MyControllerBase. Ob dieser Entwurf gut oder schlecht ist, hängt nur davon ab, ob Sie möchten

  • stellen Sie sicher TheContext, SomeServiceund AnotherServicesind immer alle mit Mock - Objekte initialisiert, oder alle von ihnen mit realen Objekten
  • oder, um sie mit verschiedenen Kombinationen der 3 Objekte zu initialisieren (dh in diesem Fall müssten Sie die Parameter einzeln übergeben)

Die Verwendung von nur einem Parameter anstelle von 3 im Konstruktor kann daher durchaus sinnvoll sein.

Das Problem ist IContext, die GetServiceMethode in der Öffentlichkeit bekannt zu machen. IMHO sollten Sie dies vermeiden, stattdessen die "Factory-Methoden" explizit beibehalten. Ist es also in Ordnung, die Methoden GetSomeServiceund GetAnotherServiceaus meinem Beispiel mithilfe eines Service-Locators zu implementieren ? IMHO das hängt davon ab. Solange die IContextKlasse nur eine einfache abstrakte Factory für den speziellen Zweck der Bereitstellung einer expliziten Liste von Dienstobjekten ist, ist dies meiner Meinung nach akzeptabel. Abstrakte Fabriken sind in der Regel nur "Kleber" -Code, der nicht selbst einer Prüfung unterzogen werden muss. Trotzdem sollten Sie sich fragen, ob es im Kontext von Methoden wie GetSomeService, ob Sie wirklich einen Service Locator benötigen oder ob ein expliziter Konstruktoraufruf nicht einfacher wäre.

Wenn Sie sich also an ein Design halten, bei dem die IContextImplementierung nur eine Hülle um eine öffentliche, generische GetServiceMethode ist, mit der beliebige Abhängigkeiten durch beliebige Klassen aufgelöst werden können, gilt alles, was @BenjaminHodgson in seiner Antwort schrieb.

Doc Brown
quelle
Ich stimme dem zu. Das Problem mit dem Beispiel ist die generische GetServiceMethode. Das Umgestalten auf explizit benannte und typisierte Methoden ist besser. Besser wäre es noch, die Abhängigkeiten der IContextImplementierung explizit im Konstruktor zu formulieren.
Benjamin Hodgson
1
@BenjaminHodgson: Es mag manchmal besser sein, aber es ist nicht immer besser. Sie bemerken den Codegeruch, wenn die ctor-Parameterliste immer länger wird. Siehe meine frühere Antwort hier: programmers.stackexchange.com/questions/190120/…
Doc Brown
@DocBrown Der "Codegeruch" der Überinjektion von Konstruktoren weist auf eine Verletzung der SRP hin, was das eigentliche Problem ist. Einfach mehrere Dienste in eine Fassadenklasse Einwickeln, nur um sie als Eigenschaften zu belichten, tut nichts , die Quelle des Problems. Daher sollte eine Fassade keine einfache Hülle für andere Komponenten sein, sondern im Idealfall eine vereinfachte API bieten, mit der sie verwendet werden kann (oder auf andere Weise vereinfacht werden kann) ...
AlexFoxGill
... Denken Sie daran, dass ein "Codegeruch" wie eine Überinjektion des Konstruktors an sich kein Problem darstellt. Es ist nur ein Hinweis darauf, dass es ein tieferes Problem im Code gibt, das normalerweise durch eine sinnvolle
Umgestaltung
Wo waren Sie, als Microsoft dies einbaute IValidatableObject?
RubberDuck
15

Dieses Design ist als Service Locator * bekannt und gefällt mir nicht. Es gibt viele Argumente dagegen:

Service Locator koppelt Sie an Ihren Container . Mit der regulären Abhängigkeitsinjektion (bei der der Konstruktor die Abhängigkeiten explizit angibt) können Sie Ihren Container direkt durch einen anderen ersetzen oder zu new-expressions zurückkehren. Mit deinem ist IContextdas nicht wirklich möglich.

Service Locator verbirgt Abhängigkeiten . Als Client ist es sehr schwierig zu sagen, was Sie zum Erstellen einer Instanz einer Klasse benötigen. Sie benötigen eine Art von IContext, aber Sie müssen auch den Kontext einrichten, um die richtigen Objekte zurückzugeben, damit die MyControllerBaseArbeit ausgeführt werden kann. Dies ist aus der Unterschrift des Konstrukteurs gar nicht ersichtlich. Mit dem regulären DI sagt Ihnen der Compiler genau, was Sie brauchen. Wenn Ihre Klasse viele Abhängigkeiten hat, sollten Sie diesen Schmerz spüren, weil er Sie zur Umgestaltung anspornt. Service Locator verbirgt die Probleme mit schlechten Designs.

Service Locator verursacht Laufzeitfehler . Wenn Sie GetServicemit einem schlechten Typparameter aufrufen, erhalten Sie eine Ausnahme. Mit anderen Worten, Ihre GetServiceFunktion ist keine Gesamtfunktion. (Total Functions sind eine Idee aus der FP-Welt, aber es bedeutet im Grunde, dass Funktionen immer einen Wert zurückgeben sollten.) Lassen Sie sich besser vom Compiler helfen und sagen, wenn Sie die falschen Abhängigkeiten haben.

Service Locator verstößt gegen das Liskov-Substitutionsprinzip . Da sein Verhalten vom Typ des Arguments abhängt, kann der Service Locator so betrachtet werden, als ob er eine unendliche Anzahl von Methoden auf der Oberfläche hat! Dieses Argument wird im Detail dargelegt hier .

Service Locator ist schwer zu testen . Sie haben ein Beispiel für eine Fälschung IContextfür Tests angegeben, was in Ordnung ist, aber es ist sicherlich besser, diesen Code nicht erst schreiben zu müssen. Fügen Sie einfach Ihre gefälschten Abhängigkeiten direkt ein, ohne über Ihren Service Locator zu gehen.

Kurz gesagt, tu es einfach nicht . Es scheint eine verführerische Lösung für das Problem der Klassen mit vielen Abhängigkeiten zu sein, aber auf lange Sicht werden Sie Ihr Leben nur unglücklich machen.

* Ich definiere Service Locator als Objekt mit einer generischen Resolve<T>Methode, die beliebige Abhängigkeiten auflösen kann und in der gesamten Codebasis verwendet wird (nicht nur im Kompositionsstamm). Dies ist nicht dasselbe wie Service Facade (ein Objekt, das einige bekannte Abhängigkeiten bündelt) oder Abstract Factory (ein Objekt, das Instanzen eines einzelnen Typs erstellt - der Typ einer Abstract Factory ist möglicherweise generisch, die Methode jedoch nicht). .

Benjamin Hodgson
quelle
1
Sie erklären sich mit dem Service Locator-Muster einverstanden (dem ich zustimme). Tatsächlich ist das OP-Beispiel MyControllerBasejedoch weder an einen bestimmten DI-Container gekoppelt, noch ist dies "wirklich" ein Beispiel für das Anti-Pattern "Service Locator".
Doc Brown
@ DocBrown Ich stimme zu. Nicht weil es mir das Leben erleichtert, sondern weil die meisten der oben aufgeführten Beispiele für meinen Code nicht relevant sind.
LiverpoolsNumber9
2
Für mich ist das Markenzeichen des Service Locator Anti-Patterns die generische GetService<T>Methode. Das Auflösen beliebiger Abhängigkeiten ist der eigentliche Geruch, der im Beispiel des OP vorhanden und korrekt ist.
Benjamin Hodgson
1
Ein weiteres Problem bei der Verwendung eines Service Locator besteht darin, dass die Flexibilität verringert wird: Sie können nur eine Implementierung pro Service-Schnittstelle verwenden. Wenn Sie zwei Klassen erstellen, die auf einem IFrobnicator basieren, aber später entscheiden, dass eine Ihre ursprüngliche DefaultFrobnicator-Implementierung verwenden soll, die andere jedoch einen CacheingFrobnicator-Dekorator verwenden soll, müssen Sie den vorhandenen Code ändern Abhängigkeiten direkt einzufügen ist alles, was Sie tun müssen, Ihren Setup-Code zu ändern (oder die Konfigurationsdatei, wenn Sie ein DI-Framework verwenden). Dies ist also eine OCP-Verletzung.
Jules
1
@DocBrown Mit dieser GetService<T>()Methode können beliebige Klassen angefordert werden: "Diese Methode überprüft den aktuellen DI-Container der Anwendung und versucht, die Abhängigkeit aufzulösen. Ich denke, ein angemessener Standard." . Ich habe auf Ihren Kommentar oben in dieser Antwort geantwortet. Dies ist 100% ein Service Locator
AlexFoxGill
5

Die besten Argumente gegen das Anti-Pattern von Service Locator werden von Mark Seemann klar formuliert, sodass ich nicht zu sehr darauf eingehen werde, warum dies eine schlechte Idee ist auch Marks Buch empfehlen ).

Um die Frage zu beantworten, lassen Sie uns Ihr aktuelles Problem erneut darlegen :

Anstatt also für jeden Dienst mehrere Parameter im Konstruktor hinzuzufügen (da dies für die Entwickler, die die Anwendung erweitern, sehr lästig und zeitaufwändig wird), verwende ich diese Methode, um Dienste abzurufen.

Es gibt eine Frage, die dieses Problem in StackOverflow behebt . Wie in einem der Kommentare dort erwähnt:

Die beste Bemerkung: "Einer der wunderbaren Vorteile von Constructor Injection besteht darin, dass Verstöße gegen das Prinzip der Einzelverantwortung auffällig sichtbar werden."

Sie suchen an der falschen Stelle nach einer Lösung für Ihr Problem. Es ist wichtig zu wissen, wann eine Klasse zu viel tut. Ich vermute in Ihrem Fall stark, dass kein "Base Controller" benötigt wird. Tatsächlich ist in OOP fast immer überhaupt keine Vererbung erforderlich . Variationen im Verhalten und in der gemeinsamen Funktionalität können vollständig durch geeignete Verwendung von Schnittstellen erzielt werden, was normalerweise zu besser faktorisiertem und gekapseltem Code führt - und keine Notwendigkeit, Abhängigkeiten an übergeordnete Konstruktoren weiterzugeben.

In allen Projekten, an denen ich gearbeitet habe, gab es einen Basis-Controller, der ausschließlich dazu diente, praktische Eigenschaften und Methoden wie IsUserLoggedIn()und mit anderen zu teilen GetCurrentUserId(). STOP . Dies ist ein schrecklicher Missbrauch der Erbschaft. Erstellen Sie stattdessen eine Komponente, die diese Methoden verfügbar macht, und nehmen Sie eine Abhängigkeit davon, wo Sie sie benötigen. Auf diese Weise bleiben Ihre Komponenten testbar und ihre Abhängigkeiten werden offensichtlich.

Abgesehen von allem anderen würde ich bei Verwendung des MVC-Musters immer dünne Controller empfehlen . Sie können hier mehr darüber lesen , aber der Kern des Musters ist einfach: Controller in MVC sollten nur eines tun: Argumente verarbeiten, die vom MVC-Framework übergeben werden, und andere Belange an eine andere Komponente delegieren. Dies ist wiederum das Prinzip der Einzelverantwortung bei der Arbeit.

Es wäre wirklich hilfreich, Ihren Anwendungsfall zu kennen, um ein genaueres Urteil abzugeben, aber ich kann mir ehrlich gesagt kein Szenario vorstellen, in dem eine Basisklasse gut faktorisierten Abhängigkeiten vorzuziehen ist.

AlexFoxGill
quelle
+1 - Dies ist eine neue Einstellung zu der Frage, die keine der anderen Antworten wirklich angesprochen hat
Benjamin Hodgson
1
"Einer der wunderbaren Vorteile von Constructor Injection besteht darin, dass Verstöße gegen das Prinzip der Einzelverantwortung offensichtlich werden." Wirklich gute Antwort. Stimmen Sie dem nicht zu. Mein Anwendungsfall ist, komischerweise, dass ich keinen Code in einem System duplizieren muss, das (mindestens) über 100 Controller verfügt. Jedoch hat bezüglich SRP - jeder injizierte Dienst eine einzelne Verantwortung, ebenso wie der Controller, der sie alle verwendet - wie würde man das refraktieren ??!
LiverpoolsNumber9
1
@ LiverpoolsNumber9 Der Schlüssel besteht darin, die Funktionalität von BaseController in Abhängigkeiten zu verschieben, bis in BaseController nichts mehr übrig ist, außer einzeilige protectedDelegierungen an die neuen Komponenten. Dann können Sie BaseController verlieren und diese protectedMethoden durch direkte Aufrufe von Abhängigkeiten ersetzen - dies vereinfacht Ihr Modell und macht all Ihre Abhängigkeiten explizit (was in einem Projekt mit Hunderten von Controllern eine sehr gute Sache ist!)
AlexFoxGill
@ LiverpoolsNumber9 - Wenn Sie einen Teil Ihres BaseControllers in einen Pastebin kopieren könnten, könnte ich einige konkrete Vorschläge machen
AlexFoxGill
-2

Ich füge eine Antwort hinzu, die auf den Beiträgen aller anderen basiert. Vielen Dank an alle. Hier ist zunächst meine Antwort: "Nein, daran ist nichts auszusetzen."

Doc Browns "Service Facade" -Antwort Ich habe diese Antwort akzeptiert, weil ich nach einigen Beispielen oder einer Erweiterung meiner Arbeit gesucht habe (wenn die Antwort "Nein" war). Er gab dies an, indem er vorschlug, dass A) es einen Namen hat und B) es wahrscheinlich bessere Möglichkeiten gibt, dies zu tun.

Benjamin Hodgsons "Service Locator" -Antwort So sehr ich das Wissen schätze, das ich hier gewonnen habe, ist das, was ich habe, kein "Service Locator". Es ist eine "Servicefassade". Alles in dieser Antwort ist richtig, aber nicht für meine Umstände.

Antworten der USR

Ich werde das genauer angehen:

Auf diese Weise geben Sie viele statische Informationen auf. Sie verschieben Entscheidungen auf die Laufzeit, wie dies bei vielen dynamischen Sprachen der Fall ist. Auf diese Weise verlieren Sie die statische Verifizierung (Sicherheit), Dokumentation und Werkzeugunterstützung (automatische Vervollständigung, Umgestaltung, Suche nach Verwendungen, Datenfluss).

Ich verliere kein Werkzeug und keine "statische" Eingabe. Die Servicefassade gibt das zurück, was ich in meinem DI-Container konfiguriert habe, oder default(T). Und was es zurückgibt, ist "getippt". Die Reflexion ist eingekapselt.

Ich verstehe nicht, warum das Hinzufügen zusätzlicher Dienste als Konstruktorargumente eine große Belastung darstellt.

Es ist sicherlich nicht "selten". Als ich eine bin mit Basis - Controller, ich jedes Mal müssen sie Konstruktor ändern, kann ich 10 ändern müssen, 100, 1000 anderen Controllern.

Wenn Sie ein Abhängigkeitsinjektionsframework verwenden, müssen Sie die Parameterwerte nicht einmal manuell übergeben. Andererseits verlieren Sie einige statische Vorteile, aber nicht so viele.

Ich benutze Abhängigkeitsinjektion. Das ist der Punkt.

Und schließlich, Jules 'Kommentar zu Benjamins Antwort, verliere ich keine Flexibilität. Es ist meine Servicefassade. Ich kann so viele Parameter hinzufügen, GetService<T>wie ich zwischen verschiedenen Implementierungen unterscheiden möchte, wie dies bei der Konfiguration eines DI-Containers der Fall wäre. So zum Beispiel könnte ich ändern , GetService<T>()um GetService<T>(string extraInfo = null)dieses „potenzielles Problem“ zu umgehen.


Trotzdem nochmals vielen Dank an alle. Es war wirklich nützlich. Prost.

LiverpoolsNumber9
quelle
4
Ich bin nicht einverstanden, dass Sie hier eine Servicefassade haben. Die GetService<T>()Methode ist in der Lage (zu versuchen), beliebige Abhängigkeiten aufzulösen . Dies macht es zu einem Service Locator und nicht zu einer Service Facade, wie ich in der Fußnote meiner Antwort erklärt habe. Wenn Sie es durch eine kleine Menge von GetServiceX()/ GetServiceY()Methoden ersetzen würden, wie @DocBrown vorschlägt, wäre es eine Fassade.
Benjamin Hodgson
2
Sie sollten den letzten Abschnitt dieses Artikels über den Abstract Service Locator lesen und genau beachten. Dies ist im Wesentlichen das, was Sie tun. Ich habe gesehen, dass dieses Anti-Pattern ein ganzes Projekt korrumpiert - achten Sie besonders auf das Zitat "Es wird Ihr Leben als Wartungsentwickler verschlechtern, weil Sie beträchtliche Mengen an Gehirnleistung benötigen, um die Auswirkungen jeder von Ihnen vorgenommenen Änderung zu erfassen "
AlexFoxGill
3
Bei der statischen Eingabe fehlt Ihnen der Punkt. Wenn ich versuche, Code zu schreiben, der mit DI keine Klasse mit einem erforderlichen Konstruktorparameter bereitstellt, wird er nicht kompiliert. Das ist statische Sicherheit während der Kompilierung gegen Probleme. Wenn Sie Ihren Code schreiben und Ihrem Konstruktor einen IContext bereitstellen, der nicht richtig konfiguriert wurde, um das Argument bereitzustellen, das er tatsächlich benötigt, wird er kompiliert und schlägt zur Laufzeit fehl
Ben Aaronson
3
@ LiverpoolsNumber9 Na warum haben Sie dann überhaupt eine stark typisierte Sprache? Compilerfehler sind eine erste Verteidigungslinie gegen Fehler. Unit-Tests sind die zweite. Ihre würden auch nicht von Unit-Tests erfasst, da es sich um eine Interaktion zwischen einer Klasse und ihrer Abhängigkeit handelt, sodass Sie sich in der dritten Verteidigungslinie befinden würden: Integrationstests. Ich weiß nicht, wie oft Sie diese ausführen, aber Sie sprechen jetzt über Feedback in der Größenordnung von Minuten oder mehr und nicht über die Millisekunden, die Ihre IDE benötigt, um einen Compilerfehler hervorzuheben.
Ben Aaronson
2
@LiverpoolsNumber9 falsch: Ihre Tests müssen jetzt a IContextausfüllen und das einspeisen, und entscheidend: Wenn Sie eine neue Abhängigkeit hinzufügen, wird der Code immer noch kompiliert - obwohl die Tests zur Laufzeit fehlschlagen
AlexFoxGill