Kategorien
Code Reviews – Der aktuelle Stand
Heute wollen wir mal ein Thema ansprechen, bei dem ich mal wieder auf eine Diskussion hoffe. Eine Diskussion die dazu dient Erfahrungen und Best Practices auszutauschen. Grundsätzlich geht es dabei um Code Reviews. Ich will „kurz“ erklären, wie wir sie zur Zeit durchführen und hoffe, dass ihr dann eure Vorgehensweisen beschreibt. Aber fangen wir doch einfach mal an.
Wie wohl die meisten anderen auch, fangen wir mit der Programmierung von Code an, da sitzen dann ein bis zwei Personen am PC und produzieren Source-Code, der dann ins SVN eingecheckt wird. Sobald ein Stand erreicht ist, dem ein Review gut tun würde, wird bescheid gegeben und eine Person drauf angesetzt. Dieser Reviewer checkt dann lokal das Repository aus und geht den Code Zeile für Zeile durch.
Bis jetzt könnte ich mir vorstellen, dass es bei allen noch ähnlich aussieht. Jetzt kommen wir zur Dokumentation der gefundenen Smells, Fehler oder sonstigem. Wir haben uns hier darauf geeinigt über PHPDoc-Kommentare zu gehen. Ein solcher Kommentar könnte wie folgt aussehen:
// @review 2010-07-05 NLA Don't use standard exceptions
Da wir alle Eclipse, bzw. Zend Studio bei uns im Team verwenden können wir, da einen Task Tag @review haben schön nach diesen Stellen suchen und sie uns auch einfach sortieren lassen (falls es jemanden interessiert, wie man solche Tags erstellt, möge er jetzt sprechen). Wenn alles mit Kommentaren versehen ist, wird der Code wieder eingecheckt. Der oder die Programmierer, können sich das Review dann anschauen und gegebenenfalls drauf reagieren.
Wenn man nicht einer Meinung ist und das wird öfters passieren setzen wir und danach noch zu einer kleinen Diskussionsrunde zusammen. Alles immer sehr informell. Um ehrlich zu sei bin ich gespannt drauf, wie ihr Reviews betreibt. Wir haben gute Erfahrungen mit dem System gemacht und finden es relativ effizient. Ich kenne aber diverse Reviewtechniken, die eher in der großen Runde ablaufen. Laut Code Complete sollten diese Verfahren sehr effizient sein, aber irgendwie kenne ich niemanden, der sie anwendet.
Wir nutzen Peer Reviews und führen die zusammen durch. D.h. der Entwickler und ein Kollege schauen sich den Code an. Änderungen werden per TODO im Code markiert. Alles auch ganz informell.
Ein Code Review gehört bei uns zum done done state.
Finde beide Ansätze ganz gut.
Für Nils‘ Version spricht, dass man am Ende gut unterscheiden kann, was wirkliche Änderungsvorgaben (TODO) sind und was nur Anregungen sind.
Bei Nobert’s Variante ist das wieder eine andere Situation, da Entwickler und Reviewer zusammen da sitzen und der Entwickler schon sagen kann, ob das so sinnvoll ist, es zu ändern, oder ob er sich was dabei gedacht hat, was beim Reviewer so nicht direkt angekommen ist (oder zu schlecht kommentiert / dokumentiert ist?).
Ich finde trotzdem Nils‘ Variante unterm Strich besser, da der Reviewer so einfach „freier“ an die Sache geht und wenn nicht zwischendrin ein Entwickle ist, mit dem man schon über gewisse Sachen redet, bekommt der Reviewer auch ein besserers Gefühl, ob eventuell Stellen doch nicht ganz durchsichtig sind.
Ich habe das bisher auch immer mit dem // TODO Tag markiert, wobei mir die Idee mit dem eigenen „review Tag“ ehrlich gesagt ziemlich gefällt.
Auf jeden Fall würde mich aber auch die „Software Variante“ interessieren, die von Smartbear so „propagiert“ wird.
Eventuell für den ein oder anderen interessant: http://ragtek.org/blog/allgemeines/buchtipp-code-reviews-best-kept-secrets/
Grundsätzlich halte ich von >todo< im Code überhaupt nichts.
Meistens bleiben die liegen oder sind schlecht dokumentiert. Nunja, mit Disziplin kann man beide Probleme vermeiden.
Bei uns wurde ein Tool für Eclipse namens Jupiter (http://code.google.com/p/jupiter-eclipse-plugin/) vorgestellt, was recht vielversprechend aussieht. Damit markiert man zu reviewende Stellen und Nacharbeiten code-nah aber nicht im Code. Jupiter bringt dann per se ein Verfahren zum Review mit.
Allgemein gestalten wir Code-Reviews sowieso schon zu dritt.
Ein Entwickler, ein Reviewer und ein Moderator.
Aktuell gibt es bei uns noch keinen definierten Review-Prozess.
Der von Nils vorgestellte Prozess gefällt mir auch sehr gut.
Zu den @todo-Tags… Wenn man ein Continuous-Buildsystem hat und im Checkstyle die Regel aktiviert hat, dass es diese @todo-Tags anmeckern soll so tauchen die auch in der Statistik und der Auswertung auf. Wichtig ist das mindestens eine Person regelmässig durch den Code geht und diese Probleme beseitigt. Das ist dann eine gute Gelegenheit zu überlegen ob das Todo noch aktuell ist oder eh nie umgesetzt wird.
Ich finde diese Todos sehr schön, weil es wie kleine gelbe Notizzettel im Code sind. Oft fält einem mal etwas ein und man schreibt es rein. Besser als das Dinge immer unter gehen.
So sehe ich das! =)
Wir machen gar keine reviews, das macht mich sehr sehr traurig.
Indyk – willkommen im Club. Bei uns gibts sowas auch nicht. Die größte Review die wir jemals hatten war ein „Was hast du denn DA zusammenprogrammiert?“.
Ich frag mich aber auch wo ihr die Zeit für sowas her nehmt … ein Programmierer der sich hinsetzt und das System Zeile für Zeile durchgeht und kommentiert? Sollte es solch paradiesische Sachen tatsächlich geben???
Also reviews machen wir auch, nur nicht direkt im Code. Für mich gehört das auch nicht in den Code bzw. ins SVN.
Wir drucken kritischen Code meist aus und gehen den dann durch.
@Sebastian und @Norbert
Ich würde auch nicht unbedingt @todo und @review unter dem @todo-Topf vermischen. Das wird sonst schnell zu einem undurchsichtigen Durcheinander. Ich kenne viele Projekte, in denen es Tausende @todo gibt. Da wäre eine solche Unterscheidung von Anfang an heute sehr dienlich. Vielleicht sogar noch ein @critical oder so dazuerfinden.
@m.a.
Ist zwar vielleicht altklug, aber man schreibe sich immer wieder hinter die Ohren (auch ich mir selbst): „If you want to go fast, go well.“ Dann hat man auch Zeit für Code-Reviews.
@mario: Denkt denn keiner an die armen Bäume?
@Kim: You are preaching to the choir. Das Problem ist nicht, dass ich das nicht will, das Problem ist, dass der Chef nicht einsieht, dass zwei oder gar drei Programmierer stundenlang rum sitzen und sich (scheinbar) an den Füßen spielen. Die Erfahrung zeigt leider dass es dem Kunden (und ultimativ damit dem, der den Quatsch bezahlt) total egal ist ob der Code seiner Applikation elegant und sauber strukturiert ist, solange der ganze Kram läuft. Interessant ist das nur für uns, weil wir uns auch noch in einem Jahr mit dem Mist beschäftigen müssen den wir selbst verzapft haben. Für Kunden (und damit für Chef) entsteht durch sauberen Code kein Mehrwert. Zumindest keiner, den man ihnen plausibel machen kann.
Ist leider meine Erfahrung.
@ Mathias Methner: Das Problem das ich dort sehe, ist aber, dass dann jeder im Team Eclipse bzw. Zend Studio (Eclipse basiert) verwenden muss. Das wäre bei uns z.B. undenkbar, da hier jeder seine Umgebung so einrichtet, wie er es am liebsten hat. Manche nutzen Netbeans, viele ZDE 5.5, manch einer vllt. auch Eclipse und der andere will zu PHP Storm. Da kann man das dann schlecht so praktizieren.
@m.a: Traditionell entfällt auf die Entwicklung wesentlich weniger Arbeitsaufwand wie auf die Wartung. Wenn von vorn herein ungünstige Stellen im Code optimiert werden, bevor sich der Flaw durchzieht und die Arbeit immer mehr wird sollte auch deinem Chef klar werden, dass Reviews Sinn machen.
Code Reviews, was sind das? 😉
Nee, mal im Ernst. Wenn ein Modul, oder ein definierbarer Bereich fertig programmiert ist, wird reviewt. Also zum Thema TODOs und/oder @irgendeineranderer Tag denke ich: TODO ist ein noch zu erledigender Abschnitt und kein Hinweis auf eine Verbesserung oder ähnliches. Das kann eine neue Funktionalität sein, oder aber auch eine noch zu implementierende Validierung etc. etc. Deshalb halte ich es persönlich für problematisch, diesen Tag für die Auszeichnung der beim Review gefundenen Codefragmente zu verwenden. Besser ist es schon, den von Nils verwendetet Review oder meinetwegen auch XXX, zu verwenden.
Ich halte es für etwas kontra-produktiv wenn der eigentliche Entwickler mit an der Beurteilung teil nimmt. Durch seine Erklärungen, warum er etwas so umgesetzt hat, wird die Beurteilung m.E. etwas geschönt. Persönliche Be- und Empfindlichkeiten spielen gerade bei Programmierern eine nicht zu unterschätzende Rolle. Und wer will sich schon die Stimmung im Team versauen?
Ob Code gut les- und dadurch leicht wartbar ist, kann aber gerade von den Menschen beurteilt werden, die ihn nicht selbst geschrieben haben. Sie merken am ehesten, ob sich schnell ein Verständnis einstellt oder eher eine immer größer werdende Fragezeichenwüste. Den Ansatz halte ich für „objektiver“. Wer die Meckereien von CodeSniffer und Co. gewohnt ist, sollte damit keine ernsthaften Schwierigkeiten haben.
Selbstverständlich soll der Entwickler die Ergebnisse kennen lernen, damit ein Lernprozess stattfinden kann oder sogar selbst ein Refactoring machen.
Von der Beurteilung würde ich ihn aber ausschließen.
Wie dokumentiert ihr denn Stellen, bei denen nach der Review gegen eine Code-Änderung entschieden wird? Bei einer möglichen späteren Review will man ja nicht wieder die selbe Geschichte diskutieren?
Ich schreibe meinen „Teil“ direkt im Code dazu.
Bei meinem ehemaligen Arbeitgeber war es so, dass wir ALLES in den Code schreiben.
Nachteil: Nach ner Weile gibt es viel zu viele Kommentare. Für den Kunden ist es uninteressant was wir uns da denken daher gab es die Idee, während dem Buildprozess die „TODO Teile“ aus zuschneiden.
Dann gibt es noch http://smartbear.com/codecollab-features.php was sich echt nett anhört, aber den Luxus hat sich noch niemand in meiner Umgebung(inkl mir) gegönt, so das ich reinschnuppern konnte.
@m.a.
Mach nen Business-Plan oder so, dann versteht es auch der Chef 😉 Irgendwo muss für die Kurzsichtigen BWL’ler ein ROI zu erkennen sein. Dann klappt’s auch mit dem Review.
Ist zwar jetzt schon ein paar Tage alt, der Artikel, aber er ist mir heute wieder zufällig in den Sinn gekommen und da wollte ich noch mal meinen heutigen Ansatz niederschreiben, rein der komplettheit zur Liebe 😉
Es ist vllt. auch ganz gut möglich ein Review in GitHub zu machen. Dort kann man direkt an den einzelnen Code Zeilen KOmmentare anbringen. Auf die Kommentare kann man wiederum eingehen und somit entsteht dort eine Diskussion direkt an den einzelnen Code Zeilen. Wenn es dann abgearbeitet ist, kann man die Markierung auch wieder entfernen.