Kategorien
Fehlende PHP_CodeSniffer Sniffs
Bevor ich heute mit dem Artikel anfange, nochmal kurz einen Dank an Sven für seinen Artikel über PTI. Heute wollen wir uns mal wieder mit einem meiner Lieblinkstools beschäftigen: dem PHP_CodeSniffer. Ihr werdet in vielen Artikels ja schon rausgehört haben, dass die statische Code-Analyse eines meiner Steckenpferde ist und der Sniffer ist hier halt eines der besten Werkzeuge.Nachdem jetzt auch noch PTI den Einzug in mein PDT gefunden hat, ist die Lust auf neue Sniffs natürlich wieder gestiegen. Das Tool ist toll, nur leider gibt es kaum PHP Entwickler, die eigenen Sniffs schreiben und diese auch wieder zurückflißen lassen. Schade schade. PCS bringt zwar in der Grundinstallation einige sehr wichtige Sniffs mit, aber wenn man wirklich den „perfekten“ Code schreiben will, dann fehlt es an der einen oder anderen Stelle noch.
Was der Sniffer wunderbar abdeckt sind Formatierungsregeln. Wo gehören Klammern hin und an welche Stellen müssen durch eine Leerzeichen geschmückt werden. Gar nicht zu bezweifeln ist, dass diese Sniffs einem helfen ein einheitliches Codebild zu generieren. Sie sind also wichtig. Ich würde aber gerne noch mehr Dinge mit dem Sniffer machen. Am liebsten Stellen im Code finden, die gefährlich, falsch oder unnötig sind. Aus diesem Grund würde ich gerne eine Art Wunschliste aufstellen, mit Sniffs, die mir persönlich fehlen. Ich habe natürlich die Hoffnung, dass ihr euch dann mal hinsetzt und sie programmiert. Vielleicht passiert es auch genau andersherum und ihr habt eine tolle Idee und ich programmiere sie euch.
- DefinedButNeverUsed Dieser Sniff findet alle Variablen, denen mal einen Wert zugewiesen bekommen haben, die aber danach nie wieder verwendet wurden. Wie es der Zufall will, habe ich den Sniff schon fertig, ich warte nur noch auf das O.K. meines Brötchengebers, dass ich es der Gemeinde „schenken“ darf.
- UsedButNeverDefined Hier prüft der CodeSniffer, ob man mit Variablen hantiert, die niemals zuvor mit einem Wert gesegnet wurden. Den Sniff habe ich auch schon umgesetzt, musste dabei aber ein wenig tricksen, wenn es um Call-by-Reference Methoden ging. Werde ich aber noch angehen und dann auch veröffentlichen.
- privateNeverUsed Dieser Sniff findet private Methoden oder Attribute, die zwar definiert, aber niemals aufgerufen oder verwendet wurden. Hier habe ich noch nicht angefangen, das darf sich also jemand von euch schnappen.
- FunctionLength In den meisten Fällen beinhalten Coding Guidelines so einen Satz wie: „Jede Methode darf höchsten x Zeilen lang sein“. Das x kann in jedem Projekt anders sein, aber es gibt keinen Zweifel daran, dass es eine natürliche Grenze gibt, was ein Programmierer noch überblicken kann. Der Sniff sollte nicht schwer sein. Eigentlich muss man da nur die \n zählen in jeder Funktion. Also ran an den Speck.
Das waren auch eigentlich erstmal die Sniffs, die mir eingefallen sind. Ich bin mir aber sicher, dass ihr auch noch eine ganze Menge Ideen habt. Würde mich auf jeden Fall freuen, wenn ihr ein wenig Brainstormed. Vielleicht kennt auch der ein oder andere ein Feature im Zend Studio oder Netbeans, dass er gerne auf der Kommandozeile auch hätte. Habe mich ja heute zum Beispiel belehren lassen, dass der privateNeverUsed Sniff schon nativ in Netbeans integriert ist.
Re: FunctionLength: Was zum Teufel?
Vor einigen Monaten wurde ich bereits schon einmal in eine Diskussion verstrickt, bei der es um eine Maximal-Anzahl von Methoden pro Klasse geht. Schon damals konnte ich nicht verstehen, warum eine Konstante das Design meiner Klasse bestimmen soll.
Dasselbe gilt für FunctionLength: Meine Funktionen sind so lange wie sie sein müssen. Kontroller-Funktionen kürzer, Model-Funktionen teilweise sehr lang und komplex*. Warum sollten sie plötzlich nur noch 17 Zeilen lang sein dürfen? Oder 23? Oder 42?
Coding Guidelines in allen Ehren, aber eine Regel muss für mich auch Sinn machen, damit ich sie befolge.
* = Ich arbeite viel mit Wetterdaten und muss zeitaufwändige Interpolationen mit vielen Ausnahmen programmieren. Keine Chance da etwas zu modularisieren oder verallgemeinern.
@Christian: Aber du willst mir nicht erzählen, dass irgendjemand eine Methode warten kann, die 400 Zeilen lang ist?! Es sagt niemand, dass die Grenze bei 17 sein soll. Ist genau das gleiche, bei verschachtelten Bedingungen ein if if if if … kann doch niemand lesen bzw. weiterentwickeln. Natürlich kann es Ausnahmen gelten (wobei mir keine einfällt). Aber grundsätzlich kann Code immer Wartbar sein. Auch wenn es um Interpolationen von Wetterdaten geht.
Eine Diskussion ist aber natürlich wie immer herzlich willkommen 🙂
Doch. Diese hochspezialisierten Methoden sind lang und komplex, dementsprechend grosszügig werden sie von mir auch kommentiert. Das Know-How das darin steckt ist zusätzlich extern in einer Spezifikation beschrieben.
Aber schlussendlich muss in meinem Fall die Performance stimmen. Da gehen dann die Schulbuch-OO-Regeln schnell mal baden.
Keine Angst, in den Grundzügen arbeite ich weiter mit MVC-Regeln und in korrekten OO-Patterns, aber eine verkrampfte Stilisierung des Codes, um aus der Luft gegriffenenen Regeln zu folgen, kann doch auch nicht das Ziel sein.
Wo würdest denn du die Grenze in diesem konkreten Fall ansetzen? 😉
Übrigens: Dein Sniffer kann nicht einfach /n’s zählen, sonst wird man ja fürs Kommentieren bestraft. Und für Spaghetti-Code auf einer Zeile belohnt. Ich denke du musst Tokens zählen.
Jap das sehe ich ein. Den Sniffer sollte man so konfigurieren, dass man entweder LOC oder NCLOC (non-comment lines of code) zählen kann. Die Idee gefällt mir sogar recht gut. Werde mal schauen, ob ich das hinbekomme. Meine Erfahrung ist übrigens, dass man bei 100 Zeilen kaum noch den Überblick behalten kann.
Ich stimme da eher christian zu und nebenbei empfinde ich CodeSniffer als einer der überbewertesten Dev-Tools überhaupt. Wenn ich Zeit damit verbringe ein Sniffer zu bauen, der eine Variable sucht, die verwendet, aber nie definiert wird, investier ich die Zeit doch lieber darin mein error_reporting-Level höher zu setzen, PHP beschwert sich dann schon von ganz allein. bei „privateNeverUsed“ fällt mir nur Code-Coverage ein, da springt einen die Methode gleich auch noch an. Kann auch durchaus Absicht sein, dass sie noch da ist, zB weil sie mal gebraucht wurde („deprecated“) und ich sie als Fallback lieber noch nicht löschen möchte
Auf der anderen Seite möchte ich mir nicht von einem Programm sagen lassen, dass ich Fehler gemacht habe, wenn ich Regeln _bewusst_ ignoriere, was öfter vorkommt, als man denkt, bzw anders herum will ich auch nicht mein Style von einem Programm diktieren lassen.
Oder wieso sollte ich Sniffs schreiben/verwenden (ich glaub, die gibs im Standard-Set schon), die Einrückungen mit Tabs oder Leerzeichen am Ende sucht? Intelligente IDEs setzen Einrückung gleich als Leerzeichen und entfernen folgende Leerzeichen beim Speichern automatisch.
Viele Konventionen sind sowieso irgendwie abstrakt. Welchen tieferen Sinn hat die One-True-Brace-Form?
Ich finde, ein CodeSniffer behindert eher, wenn ich nehmen Bugs und Features auch noch Stilbrüche suchen und beheben muss. Da kommt man ja zu nichts mehr.
@KingCrunch Das sehe ich anders. Ich halte eine einheitliche Codeformatierung wichtig, um den Code von anderen einfacher lesen zu können. Ich finde auch Methoden, die ich mal verwendet habe und jetzt nicht mehr, haben im Code nichts verloren. Das speichert das SVN für mich. Das mit dem error_reporting höher stellen klappt auch nur, wenn du per Laufzeit „zufällig“ über die Zeile Code läufst.
Ich freue mich über jede Hilfe, die mir meine IDE bei Leichtsinnsfehlern geben kann. Natürlich kann meine Code Coverage Dinge aufzeigen. Aber in vielen Projekten in die ich hineingestolpert bin hatten halt keine Code Abdeckung. In einer perfekten Welt würde das bestimmt anders aussehen. Aber genau solche Sniffs haben mir schon oft Fehler im Code gezeigt oder die Lesbarkeit um einiges erhöht.
„Ich halte eine einheitliche Codeformatierung wichtig, um den Code von anderen einfacher lesen zu können.“
Natürlich, ich auch, man kann es aber auch zu eng sehen. Wie habe ich meine Deutschlehrerin in der Grundschule gehasst, wenn es hieß, dass das große Druckbuchstaben A nicht perfekt auf zwei Zeilen lag.
—
„Ich finde auch Methoden, die ich mal verwendet habe und jetzt nicht mehr, haben im Code nichts verloren. Das speichert das SVN für mich.“
OK, issn Argument, allerdings bleibt noch, dass dies auch schon durchs Code-Coverage augenscheinlich wird. Wozu dann noch Zeit in nen Sniffer verschwenden?
—
„Das mit dem error_reporting höher stellen klappt auch nur, wenn du per Laufzeit “zufällig” über die Zeile Code läufst.“
Auch dafür sind Tests sinnvoller, das sollte jede Zeile nicht nur zufällig durchlaufen.
—
„Natürlich kann meine Code Coverage Dinge aufzeigen. Aber in vielen Projekten in die ich hineingestolpert bin hatten halt keine Code Abdeckung.“
Wärs dann nicht sinnvoller erstmal das zu klären? PHPUnit kann so ein Report sogar nebenbei generieren. Dann weiß man sogar gleich, was für Tests eventuell noch fehlen könnten.
—–
Nur um Missverständnisse zu vermeiden: Ich habe nichts Grundsätzliches gegen CodeSniffer, ich halte ihn bloss für überbewertet. Grundlegende Style-Regeln zu kontrollieren ist eine Sache.
Anders herum kann man unendlich viel Zeit in die perfekten Sniffs stecken. Da wäre die Zeit besser in ein Code-Generator investiert, denn am Ende kommt das Gleiche raus, man hat ja keinerlei Freiheit mehr.
@KingCrunch: ich gebe dir recht. Viel ist unnötig, wenn man eine gute Code Abdeckung durch seine Tests hat. Die haben wir aber nicht und es würde Jahre dauern die auch hoch zu bekommen (ca. 40 Projekte, die teilweise auf andere Frameworks aufsetzen). So einen Sniff zu schreiben dauert hingegen Stunden. Das beste ist natürlich, wenn es den Sniff schon gibt. Dann kostet es nämlich GAR KEINE ZEIT. Wenn ich also potentielle Fehler finde, ohne Zeit reinzustecken, dann mache ich das.
Na dann bringe ich mal etwas verspätet einen shameless plug an, welcher den FunctionLength Sniff enthält. Das Ganze heißt bei mir in Anlehnung an Java’s Checkstyle MethodLengthSniff.
Zu finden ist das Ganze hier. Ist jedoch schon etwas länger her das ich den implementiert habe.
Ich arbeite leider viel mit Legacy-Code den ich anpassen muss. Hab mal spaßeshalber den Zend-Standard „drüberlaufen“ lassen: ca 86.000 Fehler (hauptsächlich Zeilenlängen) und ähnlich viele Warnings.
Kann man über PTI auch einzelne Regeln (z.b. Zeilenlängen) ausschalten?
@Bastian: beim Code Sniffer kannst du deinen eigenen Standard definieren. Schau dir einfach die Datei im Standard/Zend verzeichnis an. Da die Zeilen einfach rausnehmen, die du nicht willst und als einen eigenen Standard speichern.
Ok ich bin spät dran… Aber aktuell ist das Thema trotzdem.
Generell wird viel herumgeheult wegen Coding Standards, inbesondere beschweren sich diejenigen, die schlechten Code schreiben und sich an keine Standards halten wollen.
Grundsätzlich ist es so: der Arbeit- bzw. Projektgeber ist derjenige, der die Standards festlegt (hoffentlich). Und an die hält man sich — fertig.
Wer ein Hobby-Projekt betreibt — wen kümmern da Coding Standards. Wer meint, sie nicht zu brauchen — bitte sehr. Derjenige soll dann aber nicht erwarten, dass andere den Code dann einfach verstehen oder motiviert sein werden, mitzuentwickeln.