Kürzlich habe ich eine Lösung zugeschickt bekommen, die folgende Methode enthielt.
1: public bool compare(string item1, string item2)
2: {
3: if (item1 == null || item2 == null)
4: return false;
5:
6: //more code
7: }
In diesem Webcast möchte ich einen Ansatz zeigen, den ich als gute Alternative zu obigem Code sehe. Dabei setze ich konsequent auf Objektorientierung zur Trennung der Aspekte.
Mit Tag(s) versehen: Clean Code, Video
Uli Armbruster 
Hallo,
Die Umgestaltung ist in sich schlüssig, m.E. aber overengineered 😄
Der Code in der Routine ist lesbarer, ich denke dass aber die Lesbarkeit an der Stelle des Aufrufs darunter leidet.
LikeLike
Was meinst du genau mit „an der Stelle des Aufrufs“? Meinst du den Aufrufer?
LikeLike
Ja, irgendwo verwendest Du das ja mal.
LikeLike
Gerade deshalb ist es besser wie ich finde, weil du dann erst gar nicht falsche Werte übergeben kannst. Sprich dir fliegt das dort um die Ohren, wo du den Fehler machst, z.B. wenn du einen ungültigen String erzeugst.
LikeLike
Ich finde das ist kein großer Unterschied ob ich die Fehlermeldung beim Erzeugen des CompareContext oder beim Aufruf von Compare() bekomme, wenn ein String null ist, dann ist er halt null …
Das Erzeugen des CompareContext ist m.E. aber ein zusätzlicher Overhead, der sich aufsummiert (was die Lesbarkeit betrifft) wenn man das Pattern durchgängig anwendet.
Wenn man das bei Komplexeren Funktionen mit mehr Parametern macht, ist die Lösung aber in der Tat eine Überlegung wert 😄
LikeLike
Hey Stephan,
aber das ist doch von Vorteil, wenn der Fehler beim Erzeugen des Parameterobjekts auftritt, weil auch dort falsche Werte erzeugt wurden. Wenn die Geschäftsregel besagt, dass der String nicht NULL sein darf oder dass die 2 Strings unterschiedlich sein müssen oder dass der EqualBound immer größer 0.5 sein muss, dann ist das ebene eine Geschäftsregel, die ich irgendwo implementieren muss. Fachlich gehört das für mich nicht in Compare, weil dort soll nur der Algorithmus durchgeführt werden soll. In Anbetracht des Single Responsibility Principle finde ich das sauberer. Und wenn die Geschäftsregel verletzt wird, dann macht es ja keinen Sinn, dass derjenige, der Compare implementiert hat, prüft, wie es zu dem Fehler gekommen ist, sondern derjenige, der falsche Werte reingereicht hat. Damit wäre auch ‚fail fast‘ eingehalten.
LikeLike
Dieses Video macht mich etwas unglücklich.
Ich bin ein Fan von kurzen Klassen und Methoden,
aber der CompareContext scheint mir ein wenig überflüssig. Der Null-Check verlagert sich, wie du ja selber bemerkst, und den wirst du so auch nicht los. Das Objekt könnte natürlich für alles mögliche dienen, Überschreibungen von ToString sind da durchaus valide Bedürfnisse, aber da es dafür weder einen Anwendungsfall noch einen Test gibt ist zu dem Zeitpunkt der Entwicklung die Erstellung dieser Klasse unter diesem Gesichtspunkt ein Fall von YAGNI. Wird zwar noch nirgends gebraucht, könnte man aber mal. So erkläre ich meinen Studenten Overengineering.
Das Argument, dass man mit PostSharp oder sonstigem AOP Gedöhnz da nun Design by Contract einziehen könnte, um den CompareContext vom Null-Fluch zu befreien, ist sehr zweifelhaft. Das gilt ja bereits für die ursprüngliche Signatur der Funktion und dafür muss ich die Parameter nicht erst in eine Klasse packen. Ich kann ja mit einem Aspekt wunderbar dafür sorgen, dass value1 und value2 niemals 0 sind.
Der Aspekt, dass die Implementierung von Extension-Methods auf der Context-Methode möglich wäre, ist zwar richtig, aber vollständig unsinnig. Du besitzt ja den Code – dann brauchst du auch keine Extension-Methods sondern kannst ja direkt die Implementierung ändern. Und Extension-Methods tauchen auch nicht einfach magischer Weise überall auf, sondern erfordern, dass man den Namespace importiert. Sauber in einer eigenen Datei abgelegt und weggekapselt stören die einen eigentlich nicht. Ein etwas fadenscheiniges Argument, das ich aber immer wieder höre. Namespaces, Leute.
Weiterhin ist der entstehende Code nicht wirklich objektorientiert. Effektiv geschieht der Vergleich (also die Berechnung des konkreten Jaccard-Verhältnisses) wohl in der Klasse jaccardMeassure, die innerhalb der Compare-Funktion instanziiert wird. Die Instanz bekommt in den Konstruktur keine Daten übergeben und wird auch noch lokal erzeugt, daher hat sie wohl keine Abhängigkeiten und braucht sicher auch keine Daten von Außen – dann braucht man die Klasse ja auch nicht sondern ist nur an der Methode „AreEqualByJaccardMeasure“ interessiert. Dann kann diese auch statisch sein, da kein Interesse an gehaltenem Zustand oder der Wiederverwendung der Instanz besteht.
OO ist das nicht – Funktionen, die idealerweise in erster Ordnung (d.h. ohne vorherige Instanziierung eines Datenspendenden Objektes) ansprechbar sind, weil sie von ihren Daten losgelöst behandelt werden, sind meines Erachtens nach eher der funktionalen Programmierung zuzuordnen, als der objektorientierten.
Der entstandene CompareContext ist da auch keine Ausnahme. Zwar wird ein „Objekt“ erzeugt, welches durch das Sprachkonstrukt der Klasse gebastelt wird, effektiv handelt es sich aber um eine Datenstruktur. Das Objekt hat zwei von Außen sichtbare Properties. Von Data-Hiding kann hier nicht die Rede sein. Ausserdem wird damit unnötig der Zustand gehalten und nach Außen hin verfügbar gemacht. Um für die Paarung zweier Werte zu sorgen könnte man auch ein Generisches Tupel verwenden.
Da die JaccardCompare Klasse nun nichts weiter tut, als das zuvor gepackte CompareContext-Objekt wieder auszupacken und den Inhalt in HashSets zu verwandeln, fragt sich, warum die CompareContext Klasse das nicht auch einfach selbst macht. Immerhin wären dann die die Operationen in der Nähe ihrer Daten gelagert. Dann fragt sich aber, wofür man die JaccardCompare-Klasse noch braucht.
Effektiv sitzt die Parametrisierung durch den CompareContext am Ende an der falschen Stelle. Die JaccardCompare Klasse ist bereits ein ordentliches Interface über dem JaccardMeasure-Algorithmus, da sollte auch die Datenaufbereitung sitzen.
Der Kontext ist nicht nur in dem Beispiel überflüssig, er trennt einfach nur die Daten noch weiter von ihren Funktionen. Konzeptuell ist die JaccardCompare-Klasse bereits der Kontext des Vergleichsaufrufs.
Sie erhält zwei uninterpretierte Nicht-primitive Datentypen (strings), transformiert diese und reicht sie an das eigentliche Interface weiter, welches HashSets erwartet. Ich frage mich, wie das .NET Framework aussähe, wenn an Stellen wie beispielsweise der Folgenden eine derartige Art der Parameterübergabe implementiert wäre:
http://msdn.microsoft.com/de-de/library/zkcaxw5y(v=vs.110).aspx
Nach deiner Logik bräuchte ich da ja auch eine „StringCompareContext“-Klasse um das Ignore-Case Szenario abzudeckeln.
Dein Code ist kein gutes Beispiel für Objektorientierung, da das Problem auf den Parametern von Funktionen basiert. Es wäre maximal ein Beispiel für Funktionale Programmierung, aber dafür ist es zu overengineered.
LikeLike
Hey Johannes,
sorry, komme heute nicht mehr dazu in die Diskussion einzusteigen. Ich mache mich nach den Feiertagen dran.
Schöne Feiertage,
Gruß Uli
LikeLike
Bravo, Johannes.
LikeLike
Ich finde die Lösung am Ende auch overengineert. Das if irgendwie zu kapseln, ist eine gute Idee. Aber dafür eine Klasse? Hm… Und dass dadurch im Fehlerfall etwas ehrlicher würde, sehe ich auch nicht.
compare() ist ja schon fast eine rein integrierende Methode. Das hättest du konsequent ausbauen können, indem du das if einfach nur rausziehst in eine eigene Methode, die im Erfolgsfall den Rest als Continuation aufruft. Das wäre eine Anwendung des Integration Operation Segregation Principle (IOSP) gewesen. Habe das z.B. in dieser Artikelserie beschrieben: http://geekswithblogs.net/theArchitectsNapkin/category/19719.aspx Oder auch hier: http://geekswithblogs.net/theArchitectsNapkin/archive/2014/09/13/the-incremental-architectacutes-napkin—7—nest-flows-to.aspx
Wenn die das Geschütz zu wuchtig gewesen wäre, dann vielleicht mit der kleineren Kanone Single Level of Abstraction (SLA) schießen: Dann hättest du nur die Bedingung in eine Funktion ausgelagert und das if drin stehen lassen als Einzeiler:
if (context_absent(item1, item2)) return;
Wenn jetzt ein Fehler auftritt, dann an der richtigen Stelle: nämlich hier, wo die Parameter zu einem bestimmten Zweck gebraucht werden. Ich sehe bei einem solchen Szenario, wo kein größerer Zusammenhang sichtbar ist, wo nicht klar ist, wo eine Vertrauensgrenze verläuft, keine Not, beim Aufrufer mehr Aufwand zu treiben. Wir reden ja nicht über eine Remoting-Schnittstelle, wo es vielleicht hilfreich wäre, schon vor dem Datenversand mal zu schauen, ob alles ok ist.
LikeLike
[…] Konsequente Objektorientierung – Die besseren Methodenparameter […]
LikeLike