Strukturelle Probleme zu vieler Abhängigkeiten
Während der internen Präsentation unseres DI-Frameworks kam die Frage auf, ob für zustandslose Klassen (injectables) nicht eine beliebige Anzahl weiterer injectables im Konstruktor übergeben werden könnte. In der darauf folgenden Diskussion kristallisierte sich der Grund für den Wunsch nach einer höheren Anzahl an Abhängigkeiten heraus: Die Klassen machen schlichtweg zu viel. Oder anders: Sie haben strukturelle Probleme. Zusammen mit zwei Kollegen aus der Entwicklung wurde exemplarisch eine Codestelle mit solchen strukturellen Problemen ausfindig gemacht, analysiert und refaktoriert.
Motivation
Für neue Mitarbeiter oder unerfahrene Entwickler ist es teilweise schwer zu erkennen, ob ein existierender Code gut ist. In diesem Artikel soll erarbeitet werden, woran ein Entwickler merkt, dass er auf strukturelle Probleme zusteuert oder ob er bereits welche hat. Und natürlich sollen Antworten auf die logische Folgefrage gefunden werden: Was genau ist das strukturelle Problem und wie können sie gelöst werden?
Allgemeiner Hintergrund
In den vergangenen Jahren haben wir immer mehr Wert auf eine Qualitätssteigerung in vorhandenen Quellcode gelegt. Die Testbarkeit des Codes rückte dabei immer mehr in den Vordergrund.
Die ausgesuchte Code-Stelle ist noch gar nicht so alt. Die Testbarkeit wurde hier bei der Implementierung aber leider noch nicht komplett beachtet – ein Grund, es nun besser zu machen. Im ersten Schritt wurde eine Environment-Klasse geschaffen, um die Stelle testbar zu machen. Deshalb wurden alle Abhängigkeiten in dieses Environment verschoben.
Die betroffene Codestelle
In unserem Fakturasystem werden in regelmäßigen Intervallen gebuchte Artikel automatisiert abgerechnet.
Grober Ablauf
Der CronjobController wird alle x Tage gestartet und fragt die Klasse BookingHandler nach den Buchungen, die abgerechnet werden sollen. Der BookingHandler durchsucht die Datenbank nach allen entsprechenden Einträgen in der Buchungstabelle und gibt eine Liste von BookingDataModels zurück.
Die Liste wird an die Klasse InvoiceController übergeben, der InvoiceController gruppiert die Buchungen so, wie sie am Ende auf den Rechnungen als Position geführt werden sollen.
Das gruppierte Set aus BookingDataModels wird an die Klasse InvoiceBuilder übergeben, der wiederum das InvoiceDataModel generiert.Dieses wird an den InvoiceHandler übergeben, der es in die Datenbank schreibt. Der InvoiceController aktualisiert die abgerechneten Buchungen und erstellt Log-Einträge in den entsprechenden Adressdatensätzen. So gelten die Buchungen als voll abgerechnet.
Vereinfachtes Sequenzdiagramm der betroffenen Codestelle vor dem Refactoring:
Im Refactoring lag der Fokus auf dem InvoiceBuilder, da dieser zwischenzeitlich über 10 Abhängigkeiten auf andere Klassen hatte. Bei uns sind nur drei erlaubt.
Diese hohe Anzahl an Abhängigkeiten führte dann zu dem InvoiceBuilderEnvironment, einer Klasse in die sämtliche Abhängigkeiten des InvoiceBuilders verlagert wurden, um den InvoiceBuilder überhaupt testbar zu machen. Die zu hohe Anzahl an Abhängigkeiten wurde also einfach in eine andere Klasse verschoben, das eigentliche Problem aber nicht gelöst.
Ein Test war trotzdem nur mit viel Aufwand möglich, da zum testen immer noch sämtliche Abhängigkeiten gemockt werden mussten.
Erster Ansatz für das Refactoring
Als neue Schicht zwischen dem CronjobController und dem BookingHandler wird die neue Klasse InvoiceableBookingCollectionBuilder eingeführt. Die Klasse nimmt 0-n BookingDataModels entgegen und gibt 0-n InvoiceableBookingCollections wieder zurück.
Die Aufgaben des InvoiceControllers werden geändert und nicht länger gruppiert, sondern nimmt n InvoiceableBookingCollections entgegen, die bereits gruppierte Buchungen enthalten, und rechnet diese alle ab.
Mit der neuen Klasse InvoiceableBookingCollection wird ein domänenspezifisches Datenmodell für die Klasse InvoiceBuilder eingeführt.
Ein Datenmodell für genau diesen Anwendungsfall, in SOLID-Prinzipien übersetzt, heißt: Single Responsibility wird beachtet. Single Responsibility wird oft falsch dargestellt als Klasse, die nur eine Aufgabe haben darf. In der ursprünglichen Definition bedeutet es aber: “Es darf nur einen Grund geben, die Klasse zu ändern”. Das oben eingeführte Datenmodell ist ein Beispiel dafür. Auch wenn die Klasse nur die Aufgabe hat, die Daten für Buchungen zu verwalten, ist es wichtig, dass sie hier nur zur Generierung der Rechnungen verwendet wird – es gibt also nur einen Grund sie zu ändern.
Aus diesem Datenmodell zieht sich der InvoiceBuilder alle notwendigen Informationen. Damit muss der InvoiceBuilder auf einmal keine Abhängigkeiten wie Fremdschlüssel auflösen oder Grundeinstellungen beachten.
Vereinfachtes Sequenzdiagramm der betroffenen Codestelle nach dem Refactoring:
Fazit
Ein Datenmodell darf nicht als Repräsentation einer Tabelle gesehen werden.
Um genau einen Grund zuzulassen – ein Datenmodell zu ändern – muss auch wirklich ein Datenmodell pro Aufgabe verwendet werden.
Dank der strikten Trennung von Erzeugung und Verarbeitung haben wir die Testbarkeit erhöht. An dieser kritischen Stelle entstanden vorher zu viele Abhängigkeiten und die Klasse hatte zu viele Aufgaben. Durch die Einführung des spezifischen Datenmodells wurde auch die Verarbeitung deutlich vereinfacht und die Abhängigkeiten entfielen von selbst.
Die Versuchung, Environment-Klassen einzuführen, um Abhängigkeiten weg zu kapseln und über Mocking eine Testbarkeit zu erreichen, ist hoch, da der Schritt recht einfach ist. Genau das ist aber ein klares Zeichen für ein strukturelles Problem. Es ist ein Warnsignal, denn hier sollte man sich einen Überblick verschaffen und prüfen, welche Klassen gerade mindestens eine Aufgabe zu viel haben.
