Heute möchte ich über das Thema Code Reviews berichten.
Vorgeschichte
In der NZZ haben wir Ende des letzten Jahres unsere Code Repositories in Richtung Github gezügelt. Weil das Entwicklerteam in kurzer Zeit sehr stark gewachsen ist, mussten wir Wege finden, die Codequalität auf einem hohen Stand zu etablieren. Dies haben wir mit Code Reviews und Pull Requests auf Github versucht.
Der Prozess
Ein neues Feature/Bugfix wird in einem neuen Branch, vorzugsweise mittels Pairprogramming, entwickelt. Sind der/die Entwickler der Meinung, dass Feature sei fertig, wird auf Github ein so genannter Pull Request eröffnet. Ein Pull Request zeigt die Unterschiede zwischen 2 Branches an (z.B. zwischen dem develop und dem feature Branch). Der Reviewer (Auch ein Entwickler, aber keiner der das Feature implementiert hat), macht nun folgende Dinge.
- Unittests durchlaufen lassen
- Stylechecks durchlaufen lassen
- Reviewen der Differenzen vom Pull Request
- Feedback auf der Github Plattform bezüglich Mängeln
Ist alles ok, integriert der Reviewer den Code in den develop Branch. Gibt es Mängel, sind die Entwickler wieder in der Pflicht und müssen die Mängel beheben. Dieses Ping Pong Spiel geht so lange weiter, bis der Reviewer der Meinung ist, dass alles ok ist.
Fazit
Als wir die Idee mit den Codereviews auf Github hatten, dachten wir, dass dies eine Verbesserung der Codequalität bringen könnte.
Der Effekt war aber deutlich grösser als erwartet. Die Qualität hat sich massiv gesteigert und die Knowhowverteilung ist auch besser geworden, da nun immer 2-3 Leute den Code kennen.
Da ich selber auch Reviewer bin, kann ich folgende Dinge dazu sagen:
- Code-Reviews haben sehr positive Auswirkungen auf die innere Qualität eines Projektes
- Code reviewen ist sehr streng/anspruchsvoll
- Entwickler lernen sehr viel aus den Feedbacks
- Als Reviewer lernt man selber auch sehr viel dazu
- Knowhow verteilt sich noch besser
- Checkstylefehler, schlechte Variablennamen usw. sind einfach zu finden. Softwaredesignfehler sind aber sehr schwierig auszumachen und werden häufig zu wenig genau unter die Lupe genommen
- Wenn man es seriös macht, benötigen Code Reviews sehr viel Zeit. Wenn man also unter Zeitdruck ist, leidet häufig auch die Qualität der Reviews
Was denkt Ihr über Code Reviews? Sind diese sinnvoll und wenn ja, warum? Oder sind sie nur Arbeitsbeschaffung?
ist halt immer so die Frage wie viel zeit man mit sowas verbringen darf. Es ist super das 3 Entwickler den Code kennen, die Frage ist nur, ob der Kunde dies auch zahlen möchte. ich halte dieses vorgehen nur bedingt im Agenturarbeit für praktikabel. Ich stimme dir aber völlig zu, dass man wenn man nicht grade an einem “Wegwerfprodukt” arbeitet sicher so die besten Resultate erzeugt.
Yeah, das klingt leider für mich auch nur nach einem “so möchten wir gerne arbeiten”. Es kann gut sein, dass das funktioniert, wenn man an einem Produkt arbeitet (ein CMS, ein Shopsystem); bei Individualentwicklungen zahlt das leider keiner (zumindest keiner von unseren Kunden …).
@chp & @tim: 100% Agreement ! Zwischen dem Alltag im Büro und der schönen sauberen Lehrbuchrealität gibt es extreme Unterschiede. Leider. Ich frag mich sowieso manchmal was für krasse Arbeitgeber viele Entwickler haben… Wenn ich höre für was ihr so Zeit habt bzw. bekommt… Neid !
@tim @chp @Chris
Danke für die Feedbacks.
Mir ist absolut klar, dass die Arbeit in einer Agentur schon anders ist, als wenn man an einem Produkt arbeitet. Ich möchte im folgenden Abschnitt aber ganz unverblümt antworten.
Genau solche Feedbacks zeigen mir, dass unsere Berufsgattung nach wie vor ziemlich unprofessionell ist. Ich sage nicht, das alle so sind, aber wohl viele.
Praktisch überall wo etwas hergestellt, produziert, geschöpft wird, gibt es Qualitätskontrollen. Warum gibt es das bei der Softwareentwicklung häufig nicht? Weil wir so unglaublich gut sind? Nein, weil wir kurzfristig Geld sparen wollen, mit der Begründung, dass der Kunde das nicht zahlen würde. Dabei ist mir klar, dass diese Anweisung häufig vom Chef kommt. Aber sind wir es uns nicht auch ein bisschen schuldig uns gegen diese Anweisungen zu wehren? Wenn es der Chef gar nicht begreifen will, könnte man auch einen Jobwechsel in Erwägung ziehen.
Ich bringe immer wieder gerne das Beispiel von Clean Code. Würde ein Arzt sich von einem Patienten verbieten lassen, sich die Hände zu waschen, nur weil der Patient so Zeit und Geld sparen könnte? Ich glaube wohl kaum. Warum lassen wir uns also immer alles von den Kunde vorschreiben, obwohl wir die Profis in unserem Bereich sind?
Die kurzfristige Denkweise mit dem Geld sparen hat folgenden Effekt. So lange man nichts mehr anfassen muss, ist soweit alles ok. Aber wenn man wieder mal etwas erweitern muss, trifft man Code an, der qualitativ meistens nicht gut ist (wegen fehlender Zeit, Budget und unter anderem auch Reviews). Erstens nervt man sich dann als Entwickler und zweitens kostet das den Auftraggeber dann deutlich mehr Geld, weil es länger geht bis das Feature implementiert, bzw der Fehler gefunden ist.
Natürlich kann das auch ein Geschäftsmodell sein. Zuerst günstig einsteigen und dann bei den Nachbesserungen das Geld verdienen.
In der Tat lässt sich so gutes Geld verdienen. Solange man den Kunden vernünftig beraten hat und ihm erklärt hat, warum Qualität Geld kostet; er es aber nicht bezahlten wollte, muss man wohl auch kein schlechtes Gewissen haben …
@chp: das fördert aber etwas anderes in unserer Branche …
JEDER lügt!
Das ist die Wahrheit! Wenn ich als Firma eine Ausschreibung mache, ich möchte diese Liste an Features in einer Software umgesetzt haben, gibt es im Prinzip keinen, der sagt, er schaffe nur 80% der wichitgsten Dinge, sondern jeder erzählt, um wieviel besser als die Konkurrenz er diese Software zu 100% in time fertigstellen wird.
Absoluter bullshit! Der Auftraggeber weiß, dass gelogen wird, der Auftragnehmer weiß, dass er nie ein Projekt in time fertig bekommt und immer viel nachgearbeitet werden muss.
Das ist die Realität. Dem eigenen Chef das klar zu machen glückt manchmal, aber im Regelfall eher nicht. Einzelne Lichte in dem Bereich gibt es mittlerweile schon, die qualitiv gute Software liefern können und sich vorher auf die wichtigsten Funktionen beschränkt haben.
Wenn eine Firma selbst ihre Software entwickelt und nicht verkauft an Auftraggeber, werden heutzutage höhere Qualitätsmaßnahmen ergriffen, meist auch mit einem modernen agilen Entwicklungsprozess. Bei Agenturen habe ich bisher noch keine agilen Ansätze gesehen (die vortrefflich passen würden) sondern öfters das “klassische” Wasserfallmodell – welches wiederrum als Beispiel herangezogen (und erfunden) wurde, um darzustellen, dass solch ein starres Modell nicht funktionieren kann. (siehe “What killed waterfall could kill agile” https://gist.github.com/710960)
Ich weiß nicht ob man ein gutes Gewissen haben, wenn man ständig seine potenziellen Kunden anlügen muss.
Für mich ist das ein Kriterium bei der Auswahl der Software Projekte geworden. Wenn ich vor Projekten stehe wo “Ist sowieso veraltet, wir hätten gern andere Technologie blah blah..” gilt, dann weiss ich dass das Folgeprojekt spätestens in 3 Jahren wieder vor einem Grundaufbau steht. Dies muss nicht sein, und kostet defentiv mehr Geld als eine saubere Quellen/Projektpflege.
Gute Softwareprojekte sind in der Regel über 5-10 Jahre dauernd aktiv gepflegt. Man sollte sich auch bewusst sein dass der Code den man schreibt, oft länger lebt als man sich das vorstellt. Gerade bei OpenSource Veröffentlichungen wird oft in andere Projekte kopiert, und man findet seine ursprünglich für eine Modelleisenbahn geschriebene Funktion, in einem Autopiloten für Flugzeuge wieder.
Es lohnt sich einfach nicht heiss gestricken Code zu schreiben. Ich weigere mich mittlerweile schlechte Lösungen zu implementieren und dies ist zu einem Wertemerkmal für meine Kunden geworden.
Eine herrvoragende Funktion von Git sind “Branches”. Der folgende Artikel beschreibt wie man erfolgreich mit “Branches” arbeitet.
http://nvie.com/posts/a-successful-git-branching-model/
@Folken
Wir verwenden exakt dieses Branching Modell. Hat sich bis jetzt bestens bewährt.
Mich würde wundernehmen, wie Deine Erfahrung mit Merging ist. Warum ist in git merging so viel einfacher als in CVS/SVN? Alle Artikel reden davon, dass es so ist, aber ich habe noch keine Artikel gesehen, warum es so ist. (Leider habe ich git noch nicht eingesetzt.)