Kategorien
DER CodeSniffer-Standard
Vor kurzem hatten wir ein paar Berater im Hause. Als eine Herangehensweise haben sie auf die Verwendung eines speziellen Codesniffer-Standards gesetzt, der durch seine Regeln wichtige Kriterien verifiziert. Es gibt also Regeln, die in jedem Projekt Sinn machen. Ok, das war noch einfach. Wie wäre es, wenn wir gemeinsam einen CodeSniffer-Standart zusammenstellen, der in jedem Projekt unsaubere Stellen aufspürt. Wir suchen uns also alle wichtigen Sniffs raus, die nichts mit Formatierung und so Schlonz zu tun haben und bauen den Projekt-Standard.
Ich fange mal mit einer Brainstormingliste mit Sniffs an, die drinnen sein sollten und ihr macht dann in den Kommentaren weiter oder beschimpft mich einfach wild, weil die Idee doof ist:
- Komplexität der Methoden: Ab einer bestimmen Komplexität sollte das Tool anschlagen. Das könnte man über nPath- oder Zyklomatische-Komplixität machen.
- Definiert aber nie verwendete Variablen. Jede Variablen, die man mit einem Wert füllt und sie dann doch nicht verwendet verschmutzt den Source-Code, also raus damit.
- Verwendet, aber nie definiert. Verschmutzt nicht nur den Code, sondern ist auch meisten fehlerträchtig, bzw. ist meisten ein Fehler.
- Funktionslänge. Klassiker.
- Leere Try…catch bzw if…else Blöcke. Muss ja nicht sein.
- privateNeverUsed. Gibt es noch nich, sollte aber einfach umzusetzen sein.
So das war jetzt das Grundsetup. Ich bin mir sicher, dass ihr noch eine ganze Menge an Sniffs findet. Wenn es die noch nicht umgesetzt gibt, nicht schlimm, dann programmieren wir die einfach. Kann ja nicht so schwer sein.
Gute Idee! Stimmt schon, dass man gewisse Standards immer verwenden sollte.
Also 2 davon gibt es ja schon standardmäßig in Codesniffer:
Also leere Statements (if/else, try/catch …. ) gibt es schon in Form von:
http://pear.php.net/package/PHP_CodeSniffer/docs/1.2.2/PHP_CodeSniffer/Generic_Sniffs_CodeAnalysis_EmptyStatementSniff.html
Die zyklomatische Komplexität:
http://pear.php.net/package/PHP_CodeSniffer/docs/1.2.2/PHP_CodeSniffer/Generic_Sniffs_Metrics_CyclomaticComplexitySniff.html
einfachste Regeln sind auch wichtig, z.B. wo die geschweiften Klammern in Methodendefintionen zu sein haben (gleiche Zeile, nächste Zeile, Whitespace?). Dann wäre da noch die Zeilenlänge, double oder singlequotes um Strings, count() im For-Schleifen-Kopf verbieten, der „No-Space-After-Cast“-Sniff, der „LowervasePHPFunctions“-Sniff, um die wichtigsten zu nennen, die wir einsetzen.
es geht ja auch gar nicht darum, dass die Sniffs nicht schon existieren. Es geht nur darum, einen kompletten Standard zusammenzustellen, so habe ich es jedenfalls verstanden.
Gibt’s nen Sniff der dazu zwingt, bei mehr als 3 if/elseif/else -Anweisungen hintereinander, den Code und das Design zu überarbeiten. 😉
Gute Idee. Ich hab einiger Zeit mal alle im Paket enthaltenen Sniffs in einem Excelsheet zusammen getragen um einen Überblick zu bekommen. Bei der Gelegenheit hab ich auch alle Sniffs markiert die sich gegenseitig ausschließlich. Wenn jemand diese Liste haben möchte kann er sich gerne bei mir melden.
Warum will man alles im CodeSniffer abfackeln? Es gibt noch den PHPmd (http://phpmd.org/about.html). Wir nutzen eine Kombination aus mehrern Tools und bereiten die Informationen mit dem Hudson CI auf.
Ansonsten finde ich die Idee super und fände es auch sehr gut, wenn jemand dazu ein Repository und/oder Pear-Server anlegt, sodass die Allgemeinheit nicht nur die Ideen beisteuert, sondern dieses Regelset dann auch nutzen kann 🙂
Ich kann mir auch vorstellen, dass es den einen oder anderen Entwickler geben wird, der Interesse an der Programmierung hat und das Vorhaben so mit Sourcecode unterstützt.
@Norbert: Coole Sache! Ich bin gerade dabei, mich mit dem Hudson zu befreunden. Finde ich gut die Idee, es in ein Repo/GitHub zu packen. Wer macht den Maintainer? 🙂
Fuer symfony werden uebrigens gerade auch ein paar Regeln erarbeitet.
http://github.com/denderello/phpcs-symfony
Das Problem ist ja, dass es genügend Regelstandards gibt, nur leider haben die immer auch Formatierungsregeln drinnen. Die kann man natürlich nicht gebrauchen, wenn man einen Standard haben will, den man über alle Projekte jagen kann.
Sehr gute Idee. Vielleicht könnte man sich mal die best practices aus dem Java Bereich abgucken?
Nette Idee, aber auch im Praxistest?
– Komplexität benutzen wir schon, es bringt aber nichts. Denn uns werden 50% der Methoden als „high complexity“ gemeldet.
Liegt daran, dass jede Fehlerbehandlung auch die Komplexität erhöht. Der Automatismus kann zwischen Geschäftslogik und Fehlerbehandlung aber nicht unterscheiden. Den richtigen Grenzwert zu finden ist schwierig.
– Funktionslänge: Nette Idee. Wirklich aussagekräftig wäre das aber nur in Verbindung mit der Komplexität.
Folgendes erledigt eine geeignete IDE wie NetBeans allerdings automatisch:
– nicht verwendete Variablen.
– verwendet aber nicht definiert.
– „private“ und nicht verwendet.
Die ersten beiden gehören jedoch in die Metriken – nicht in den Coding-Standard. Warnungen im Coding-Standard bedeuten in echten Projekten schließlich, dass man den Code gar nicht einchecken darf. CodeSniffer lassen viele Leute als Commit-Hook laufen.
Die obigen Metriken sind aber keine Fehler sondern reine Vermutungen, gedacht als Hinweis für das Refactoring.
@Tom
Du lieferst aber reichlich Alibis.
Z. B.:
Eine Funktion sollte immer kurz sein können, auch, wenn Sie nur eine Reihe einfacher Anweisungen enthält, lassen auch die sich wiederum zusammenfassen.
@Kim Das ist kein Alibi. Metriken auswerten ist (provokativ ausgedrückt) „Kaffeesatzlesen für Fortgeschrittene“.
Beispiel: du musst eine komplexe, mehrstufige Abfrage auf einer XML-Struktur durchführen. Diese erfordert mehrere Schleifen und Abfragen.
An jeder dieser Stellen können Ausnahmen/Inkonsistenten auftreten.
Diese Funktion wird dir garantiert als „high complexity“ gemeldet, obwohl es nicht Deine „Schuld“ ist. Die Aufgabe ist einfach per se komplex.
Noch ein Beispiel: wir haben einen OO-Wrapper um alle GDI-Funktionen. Diese Klasse hat genauso viele Funktionen, wie die GD-Library selbst. Sie wird als „zu groß“ gemeldet. Daran kann man jedoch nichts ändern.
Beispiel: es Standardalgorithmen, in einschlägiger Literatur (z.Bsp. „Algorithmic Geometry“). Diese sind hochgradig kompliziert, doch das interessiert nicht, denn wir betrachten sie als Black-Box. Trotzdem werden sie als „high complexity“ gemeldet.
Beispiel: wir haben 1 Zeile mit einem komplizierten regulären Ausdruck, der auf jedem Treffer PHP-Code ausführt und böses Voodoo veranstaltet. Ausgerechnet dieser Fall wird uns NICHT als „high complexity“ gemeldet.
Niedrige Testabdeckung: gutes Beispiel für eine Metrik. Jedoch dauert die Berechnung tlw. mehrere Minuten. Ferner gibt es Code den man (noch) nicht testen kann. Fazit: Gut als Metrik – schlecht als Coding-Standard in CodeSniffer.
Metriken liefen gute Hinweise für ein Refactoring.
Aber es sind und bleiben wirklich nur _Hinweise_ – diese sind manchmal korrekt und manchmal nicht. Viele Metriken sind obendrein aufwendig zu berechnen und eigenen sich deshalb nicht als Commit Hook.
NetBeans hat ferner ein Plug-In, um CodeSniffer live während des Editierens auszuführen. Man stelle sich vor, was eine live berechnete NPath-Complexity da bedeuten würde. So ein Coding-Standard würde die ganze IDE lahmlegen und wäre somit nutzlos. 😉
Aus diesen Gründen sind Metriken in Coding-Standards keine gute Idee und ich plädiere dafür, beides nicht miteinander zu vermischen.
Gut geeignet für Coding-Standard sind dagegen: fehlende Dokumentation, falsche PHPDoc-Kommentare (z.Bsp. fehlerhafte @param-Annotations).
So etwas eignet sich gut als Commit-Hook.
Ich habe den CodeSniffer auch in Eclipse, er kann Warnings und Errors melden.
Errors werden auch beim PreCommit abgefangen.
Und schon hätten wir die Probleme gelöst:
– hohe Komplexität: warning
– falsche einrückung o.Ä.: error
CommitHook bricht nur bei einem error ab
@Michael Du kannst ja mal ein Beispiel für die Umsetzung eines Commit-Hooks für CodeSniffer posten. Das täte mich interessieren.
im Bezug auf den Sniffer oder allgemein?
Wir nutzen den Sniffer erst seit kurzer Zeit, muss sich alles noch bewähren, im Pre-Commit ist bisher nur eine JSure Javascript-überprüfung und der Zwang einen Kommentar abzugeben vorhanden, der Sniffer soll erst in nächster Zeit noch folgen. Die 101 Zeilen, die schon da sind, werden aber eher unleserlich, wenn ich sie hier poste.
Vorschläge?
Ich habe mal ein bisschen recherchiert. Anscheinend liefert CodeSniffer selbst ein Skript für einen CVS-Hook. Es hat sich sogar jemand die Mühe gemacht dazu ein Tutorial zu schreiben: http://blog.ronnyristau.de/2008/12/04/php-code-conventions-mit-codesniffer-und-subversion/
Ich denke, das könnte für’s erste helfen.
Allerdings: @Nils hat ja schon geschrieben, dass er etwas gegen CodeSniffer als Pre-Commit Hook hat. Also gehe ich mal stark davon aus, dass er bei seinem Vorschlag für CodeSniffer gar nicht an Hooks gedacht hat. (http://www.phphatesme.com/blog/softwaretechnik/warum-svn-pre-commit-hooks-bose-sind/)
Es sei denn, er hätte zwischenzeitlich seine Meinung geändert 😉
@Tom: Hohe Komplexität deutet auch auf schlecht abstrahierte Funktionen/Methoden hin. Das man die nicht weiter aufteilen kann, halte ich für ein Ding der Unmöglichkeit. Nehmen wir bei dein XML-Beispiel: Allein, dass da das Wort „mehrstufig“ auftaucht, deutet darauf hin, dass dort zu viel passiert.
@KingCrunch okay – das klingt interessant.
Ich möchte Dir als Gegenargument ein Beispiel liefern und wir überlegen ob es besser geht.
Ich habe hier eine Folge von XPath-Ausdrücken und ein bisschen Pseudo-Code.
Die Funktion liefert den echten Typ einer Tabellenspalte. Falls die Spalte ein Foreign-Key (=Reference) ist, macht diese Funktion dazu einen Look-Up auf der Zieltabelle (wo der echte Typ definiert ist).
function getColumnType() {
if (name() = ‚reference‘) {
$columnName = @name;
if (@table)
$table = @table;
else
$table = ../../foreign[key/@name = $columnName]/@table;
if (@column)
$column = @column;
else
$column = ./../foreign/key[@name = $columnName]/@column;
select(../../../table[@name = $table]/declaration/*[@name = $column]);
}
return name();
}
XML-Struktur: database{table*{foreign*{key*},declaration{*}}}
Aus dem XML haben wir natürlich bereits Objekte gemacht. Nun würde ich gern sehen, wie Du die XPath-Ausdrücke in einfachen OO-Code umsetzt.
Ich habe etwa 10 Verzweigungen gebraucht bei rund 50 Zeilen Code.
Dieses Beispiel soll verdeutlichen, dass hohe Komplexität nicht zwangsläufig bedeutet, dass etwas schlecht abstrahiert ist. Denn: Man kann die Methode in wenigen Zeilen Pseudocode beschreiben und den Sinn in wenigen Worten erklären. Sogar die PHP-Umsetzung ist nicht übermäßig lang.
Trotzdem ist der Code am Ende „komplex“ im Sinne der Anzahl der möglichen Pfade – weil die Aufgabe von Anfang an komplex war. Das sieht man ja bereits an den XPath-Ausdrücken.
Dass diese Pfade zum großen Teil vielleicht nur Error-Stubs sind, sieht man als Entwickler schon – das weiß aber das Tool welches die Metriken erstellt nicht.