Wie ich gestern angekündigt habe, werde ich damit beginnen mein sfImageTransformExtraPlugin anhand der Clean Code Developer Grade zu überprüfen angefangen mit dem roten.

Implizit überprüfe ich mich damit natürlich selbst.

Die gute Nachricht ist, dass ich alleine mit dem Interesse bereits den schwarzen Grad erreicht habe. Die eigentlich Arbeit beginnt aber mit dem roten Grad. Dann mal los!

Jeder Grad wird mit ein paar Prinzipien sowie einigen Praktiken beschrieben. Ich werde also versuchen, meinen Code auf die Prinzipien hin zu beurteilen und eventuelle Rafaktorisierungen zu identifizieren. Danach werde ich überprüfen, ob ich die erwähnten Praktiken bereits befolge und ggf. werde ich auch hier identifizieren, was ich besser machen kann.

Wer sich für die Quellen meines Plugins interessiert, kann auf der symfony Plugins Seite nachschauen, dort ist das Repository angegeben.

Die Prinzipien

Also dann kümmer ich mich erstmal um die Prinzipien.

Don´t Repeat Yourself (DRY)

Don’t Repeat Yourself bezieht sich nicht, wie oft angenommen, nur auf Code, sondern auch auf Datenstrukturen und die strukturelle Ebene. Denn eine Aufgabe sollte nicht zweimal unterschiedlich implementiert sein.

Nun eine Datenstruktur habe ich nicht im Plugin, da es Datenbank unabhängig ist. Aufgaben habe ich weitestgehendst an dedizierte Stellen verschoben. Aber wie finde ich heraus, ob ich Doppelungen im Code habe?

Sebastian Bergmann hat da ein nettes kleines Tool namens phpcpd geschrieben, den PHP Copy Paste Detector. Die Nutzung ist relativ simpel.

$ phpcpd .
phpcpd 1.3.1 by Sebastian Bergmann.
Found 4 exact clones with 162 duplicated lines in 7 files:
  - test/unit/lib/source/sfImageSourceHTTPTest.php:55-65
    test/unit/lib/source/sfImageSourceFileTest.php:61-71
  - lib/source/sfImageSourceDoctrine.class.php:21-85
    lib/source/sfImageSourceFile.class.php:21-85
  - lib/source/sfImageSourceDoctrine.class.php:21-87
    lib/source/sfImageSourcePropel.class.php:21-87
  - lib/transforms/sfImageAlphaMaskGD.class.php:55-77
    lib/transforms/sfImageRoundedCornersGD.class.php:55-77
2.74% duplicated lines out of 5913 total lines of code.

Wenn man sich so im Netz umschaut geben die meisten Blogs bei ihrem Code immer ein Ergebnis im Promillebereich an. Ich finde 2.74 % schon ganz ordentlich.

Aber das Ergebnis stimmt. Ich habe zwei Optimierungsmöglichkeiten in puncto DRY:

1. Die sfImageSource Klassen haben derzeit noch einigermassen viel duplizierten Code. Das kommt daher, dass ich mich entschieden habe, ein Interface zu implementieren, statt von einer (teil-) abstrakten Klasse zu erben. Aber in den verschiedenen Implementationen sind einige Teile immer wieder ähnlich wenn nicht gleich. Eventuell lohnt es sich, hier doch auf eine abstrakte Klasse zu schwenken. Das Interface würde ich aber beibehalten wollen, damit andere Entwickler auch ohne die Vererbung neue sfImageSource Klassen entwickeln können.
2. Als zweites sind tatsächlich Teile der beiden Transformationen gleich. Hier macht es vermutlich Sinn, wenn ich die sfImageRoundedCornersGD Klasse so umbaue, dass sie die sfImageAlphaMaskGD Klasse verwendet, denn es ist tatsächlich nur ein spezieller Fall einer Alpha Masken Transformation.

Bei den beiden TestCases bin ich mir allerdings nicht wirklich sicher, ob sich hier ein Refaktorisieren lohnt, bzw. ob es so sinnvoll ist..?

Keep it simple, stupid (KISS)

Das KISS Prinip kann man als Entwickler sicher nur schwer beurteilen, da man schon etwas verblendet auf seinen eigenen Code blickt.

Aber nach dem ersten Feedback von Nutzern des Plugins zu urteilen, scheint die Architektur des Plugins schnell verständlich zu sein.

Darüber hinaus habe ich während der letzten Refactorings eine ganze Menge Code wegwerfen und mit einfacheren Lösungen ersetzen können.  So konnte ich zum Beispiel das Routing so ändern können (Changeset 28653), dass es viel mehr Freiheit der Konfiguration erlaubt, dabei aber gleichzeitig mehr Funktionalität des zugrunde liegenden symfony Frameworks verwendet und somit leichter verständlich wirkt. Ich glaube hier bin ich erstmal im Reinen, auch wenn es sicher immer noch etwas einfacher ginge.

Vorsicht vor Optimierungen!

Auch bei der Optimierungsregel (Nicht machen!) sehe ich das Plugin konform. Es ist natürlich wieder eine subjektive Betrachtung, aber mein Ziel bei jedem Entwicklungszyklus ist es, nicht die Ausführungszeit zu optimieren oder so, sondern möglichst viel Funktionität des Frameworks zu nutzen, keine neuen Konzepte einzuführen oder nur die notwendigen und vor allem nutzbar zu sein, also leicht verständlich und intuitiv.

So nutze ich beispielsweise das sfCache Interface von symfony für das Speichern der generierten Bilder statt es selbst wegzuschreiben und das Routing von symfony statt selbst ein Dispatching zu bauen (so war es am Anfang in den ersten Versionen).

Favour Composition over Inheritance (FCoI)

Bei FCoI geht es um die Vermeidung von Koppelung, also dem fest Verdrahten von Funktionalitäten.

In meinem Plugin gibt es nur relativ wenig Vererbung. Die, die es gibt erbt von Klassen aus dem Framework, so wie dieses es vorgibt, vom genutzten sfImageTransformPlugin, so wie dieses es vorsieht und von PHPUnit Klassen für die TestCases. Es gibt nur eine einzelne abstrakte Klasse und die folgt den Best Practices des Frameworks.

In den meisten Fällen versuche ich so gut es mir möglich ist, per Dependency Injection Funktionalität in meine Klassen zu bringen, damit diese genutzt werden kann. Sehr stark mache ich das zum Beispiel bei der sfImageTransformator Klasse, die ihre Abhängigkeiten mittlerweile injiziert bekommt, statt sie zu erben oder fest im Code stehen hat.

Ich glaube nicht, hier mehr erreichen zu können, ohne gegen die Vorgaben des Frameworks zu verstossen und damit auch gegen die Optimierungsregel, denn solcher Code würde für symfony Entwickler nicht leicht nachvollziehbar und würde vermutlich auch zu höherer Komplexität führen.

Die Praktiken

Die CCD Grade kommen mit praktischen Tipps, deren Befolgen sicherstellen soll, dass der aktuelle Grad nicht nur einmal erreicht werden kann, sondern nachhaltig Bestand hat.

Die Pfadfinderregel beachten

Wie Pfadfinderregel zum Beispiel habe ich mit diesem Plugin wirklich verinnerlicht. Ich habe es zumindest noch nicht einmal angefasst, ohne irgendwo einen Docblock zu vervollständigen, für mehr Code Coverage zu sorgen oder etwas mehr zu dokumentieren.

Mindestens aber habe ich mir immer etwas auf die unmittelbare Todo List geschrieben, wenn ich gerade mal keine Zeit für einen endeckten Codesmell hatte.

Root cause analysis

Meine Lieblichsfrage war schon immer die Sinnfrage. Ich hasse (ernsthaft) die Frage nach konkreten Lösungen für konkrete Probleme, ohne vorher genau hingeschaut zu haben, woher das Problem überhaupt kommt.

Nach meiner Erfahrung sind die Gründe für akute Probleme meist wesentlich weiter vorne zu finden. Sei es ein konzeptioneller Fehler, eine schlechte Architektur oder ein früher Workaround, dem plötzlich Symptome erwachsen.

Ich frage mich eigentlich bei jedem Problem aber auch bei jeder Lösung, ob diese intuitiv sind, ob sie die vorhandenen Ressourcen und Funktionalitäten optimal nutzen und möglichst wenig zur Komplexität beitragen. Ebenso ist es wichtig, darauf zu achten, dass eine Lösung mit möglichst wenig Abhängigkeiten entsteht, da diese sehr schnell wieder ein root cause für weitere Probleme darstellen können.

Ein Versionskontrollsystem einsetzen

Das tue ich!

Aktuell nutze ich Subversion. Ich habe aber vor irgendwann auf Git zu wechseln. Aber momentan habe ich da keine Zeit für.

Auch in der Nutzung eines solchen Systems ist mein Plugin übrigens DRY, denn ich hoste es nicht selbst, sondern nutze das Repository des Frameworks!

Erste Refaktorisierungsmuster anwenden

Die vorgeschlagenen Refaktorisierungsmuster extract method und das Umbenennen habe ich bereits vor einigen Wochen an Hand meines Plugins beschrieben.

Leider gibt es hier für PHP keine guten unterstützenden Tools, aber das geht zur Not auch von Hand.

Täglich reflektieren

Nun ja, das tue ich tatsächlich. Zumindest bekomme ich mittlerweile die ein oder andere Supportanfrage, die mich ganz einfach dazu bringt und meine Blogposts sollten ebenso davon zeugen.

Fazit

Ich denke ich kann mir ein rotes Armband bestellen.

Die weiter oben identifizierten Refaktorisierungen sind bereits auf der Todo Liste und werden bald angegangen.

CCD · Clean Code Developer · DRY · FCoI · KISS · Pfadfinderregel · phpcpd · Refactoring · Refektion · Root Cause Analysis · Sebastian Bergmann · sfImageTransformExtraPlugin

<< Copy and Paste Detection und wie man damit umgehen kann

Jetzt werde ich auch Clean Code Developer >>