Developer Content

1. April 2020 7 Min. Lesezeit

Code Review bei BRICKMAKERS

In Qualitätsmanagement bei BRICKMAKERS wurde bereits allgemein über Maßnahmen zur Qualitätssicherung in der Software-Entwicklung gesprochen. Eine dieser Maßnahmen ist die Code Review. Im Folgenden soll näher darauf eingegangen werden, wie das Thema Code Review bei BRICKMAKERS gelebt wird. Bei allem gilt allerdings, dass zwar gängige Umstände und Praktiken erläutert werden, jedes Team aber individuelle Bedürfnisse und Anpassungen hat.

Was ist Code Review überhaupt?

Es bedeutet, dass neben dem eigentlichen Umsetzer eines Tickets mindestens eine weitere Person die Änderungen angeschaut hat. Diese Änderungen werden dabei in Bezug auf die Aufgabe des Tickets und Qualitätsaspekte geprüft. Diesen Vorgang nennen wir auch “verifizieren”.

Wozu macht man die Code Reviews?

Mit ausreichend Erfahrung erkennt man, dass eine Person alleine Dinge übersieht, vergisst oder suboptimal löst. Außerdem kann es passieren, dass sich Wissen über gewisse Bereiche der Software bei einzelnen Personen ansammelt, was das Team bereits mittelfristig in seiner Arbeit behindern wird. Dazu kommt, dass sich Best Practices im Team schwerer durchsetzen lassen, wenn man sich nicht über den neuen Code austauscht.

Während man einige wenige generische Fehler und Probleme sicherlich mit Checklisten reduzieren kann, reicht das bei weitem nicht. Dazu kommt der Aspekt, dass zwar einige Fehler im späteren Verlauf des Projekts durchaus gefunden werden, dann aber oft aufwändiger in der Behebung sind, da man die potenziellen Ursachen erst eingrenzen muss.

Daher nutzt BRICKMAKERS das 4-Augen-Prinzip, d.h. da schaut einfach zeitnah nochmal ein anderer Entwickler mit einem anderen Blickwinkel drauf. Diese Vorgehensweise ist dann automatisch auch perfekt für das Onboarding neuer Mitarbeiter in der Firma oder dem Projekt geeignet.

Was wird alles gebraucht, um Code Reviews sinnvoll betreiben zu können?

Commitment im Team, dass man immer eine Code Review durchführen möchte, also dass man sich die Zeit dafür nimmt. Bei BRICKMAKERS ist das selbstverständlich.
Mechanismus in der Ticket-Verwaltung, um Ticket und Code Reviewer zusammen zu bringen. Bei uns sind das zwei zusätzliche Status eines Tickets, “Ready for Review” und “Review in Progress”.
Commitment auf den Umfang der Code Review
Definition of Done
Testbare Akzeptanzkriterien an den Tickets
Vergleichsweise kleine Tickets, sodass man schneller Feedback bekommt
Tooling, das bedeutet bei BRICKMAKERS hauptsächlich eine Source Control wie z.B. Git
Strukturierte Vorgehensweise beim Committen von Änderungen in der Source Control

Was ist alles in einer Code Review enthalten?

Im Folgenden sind einige gängige Punkte genannt. Selbstverständlich fügt jedes Team je nach Bedarf weitere Punkte hinzu.

Von den Akzeptanz-Kriterien abgeleitete manuelle Tests der Funktionalität lokal durchführen
Automatisierte Tests ausführen (langlaufende Tests können auch von der CI übernommen werden)
Commits in der Source Control prüfen
Von den Akzeptanz-Kriterien abgeleitete manuelle Tests der Funktionalität auf einem Testsystem durchführen
Definition of Done prüfen
Dem Umsetzer Feedback geben
Je nach Branching-Strategie des Teams auch mergen

Was ist die Definition of Done?

Auch jenseits der Akzeptanzkriterien gibt es Dinge, die bei Abschluss eines jeden Tickets erfüllt sein müssen, falls anwendbar. Diese Dinge werden dann in einer Definition of Done festgehalten.

Diese wird zum Projektstart erstmalig angelegt und kontinuierlich den Bedürfnissen des Projekts angepasst.

Beispiele:

Die Akzeptanzkriterien/Features wurden manuell geprüft.
Neuer / geänderter Code ist mit automatisierten Tests abgedeckt, sofern sinnvoll.
Alle automatisierten Tests sind grün.
Dokumentation der öffentlichen Web API ist angepasst.
…

Welchen Einfluss haben Akzeptanzkriterien auf die Review?

Es kommt vor, dass man bei der Umsetzung eines Tickets feststellt, dass die Akzeptanzkriterien nicht vernünftig testbar sind, zu viel Interpretationsfreiraum lassen oder generell zu wenig festlegen. Dann muss man die Akzeptanzkriterien während der Umsetzung nochmal mehr oder weniger stark überarbeiten. Hält sich diese Überarbeitung in Grenzen, wird mit dem Commitment des Teams durchgeführt und gefährdet nicht den generellen Zeitplan, ist das vollkommen in Ordnung und auch sinnvoll. Wenn jedoch diese Probleme nicht spätestens bei der Umsetzung behoben werden, führt das im Code Review zu unnötigen Reibungen.

Die Testbarkeit von Akzeptanzkriterien kann man sicherstellen, indem man sich zwingt, sie testbar zu formulieren. Beispiel: “Wenn ich auf den Button drücke, erscheint eine Hinweismeldung mit folgendem Inhalt …”

Am gefährlichsten ist zu viel Interpretationsfreiraum. Dabei hat jeder am Ende ein anderes Bild von dem Feature im Kopf und denkt, es wäre das gleiche wie bei den anderen. Dann kann es passieren, dass ein signifikanter Teil der Umsetzung in die falsche Richtung läuft.

Wenn bei der Umsetzung zu häufig Unklarheiten auftreten, sollte man über die Qualität der Refinements nachdenken. Dort hilft es dann, die Akzeptanzkriterien testbarer zu formulieren und sich darüber Gedanken zu machen, wie der Nutzer den Workflow durchführt.

In der Praxis kann es jedoch gute Gründe haben, dass bei einem Ticket Detailaspekte noch bewusst offen gelassen werden, weil zum Beispiel Ressourcen wie Icons etc. noch nicht final vorliegen oder Dinge während der Umsetzung besser beurteilbar sind. In dem Fall ist Kommunikation im Team noch vor der Umsetzung der spezifischen Änderungen der Schlüssel. Auf diese Weise wird Verschwendung durch zu viel Refinement im Voraus vermieden.

Wie beeinflussen die Commits in der Source Control die Review?

Da der Reviewer die Qualität der Änderungen nicht nur durch manuelle Tests sicherstellt, sondern auch unter die Haube schaut, hat auch die Qualität der Commits einen starken Einfluss auf Effektivität und Dauer der Code Review.

Folgende Faktoren spielen dabei eine Rolle:

Commit-Message Zur Orientierung in der History und zum Verständnis des Lesers ist die mindestens die Benennung, also der Titel der Commit-Message, sehr wichtig. Negativbeispiel: “Fix bug”. Positivbeispiel: “The app no longer crashes when the user does not enter a password”.
Logische Abgeschlossenheit Unterschiedliche Themen in einem Commit zu vermischen, stiftet nur Verwirrung und kostet damit Zeit. Ein guter Hinweis darauf, ob ein Commit logisch abgeschlossen ist, ist der Titel des Commits. Wenn dieser zwar konkret, aber nicht unbedingt trivial ist, ist das in der Regel gut. Natürlich bedeutet das auch, dass im Commit nur das drin ist, was auch im Commit-Titel genannt ist.
Umfang Je nach Art der Änderungen in einem Commit spielt auch der Umfang eine Rolle. Bei zu vielen geänderten Dateien in einem Commit geht der Überblick verloren, was wieder Zeit kostet und dazu führt, dass der Reviewer evtl. eine wichtige Sache übersieht. Schlecht ist es also oft, ein nicht-triviales Feature in nur einem Commit zu implementieren. Als Hilfestellung kann auch hier wieder der Commit-Titel herangezogen werden, da er in diesem Fall wahrscheinlich zu schwammig ist.
Nachvollziehbare Reihenfolge Wenn Commits in einem logischen Zusammenhang zueinander stehen, erhöht das das Verständnis des Reviewers und spart damit Zeit. Außerdem hilft eine systematische Vorgehensweise gewiss auch dem Umsetzer. Schlecht ist es also z.B., wenn man viele Commits lang nur irgendwelche Klassen anlegt, ohne dass irgendein Zusammenhang zwischen ihnen besteht. Gut hingegen ist es, wenn zuerst die Nutzer einer Klasse angelegt werden und kurz darauf die Klasse selbst. So stellt man sicher, dass man nur entwickelt, was man wirklich braucht und die richtigen Abstraktionen definiert werden. Das lässt sich hervorragend mit TDD verbinden, sowohl auf End-2-End-Test- als auch auf Unit-Test-Ebene.

Wie bespricht man die Ergebnisse mit dem Umsetzer?

Es gibt mindestens vier Elemente bei der Besprechung der Code Review:

Kritik / Verbesserungsvorschläge: Ganz wichtig ist es natürlich insbesondere bei Code-Review-Neulingen, dass man Kritik und Anregungen konstruktiv unterbreitet. Das heißt, dass man respektvoll über die speziellen Änderungen spricht und sachlich argumentiert, wieso man selbst welches Problem darin sieht. Dann kann man gemeinsam in Ruhe die weitere Vorgehensweise, also wann welche Verbesserungsmaßnahmen von wem umgesetzt werden, besprechen. Am klarsten ist das natürlich, wenn die Änderungen die Akzeptanzkriterien nicht erfüllen. Aber natürlich gibt es im Gegensatz dazu auch Themen, über die sich Entwickler trotz logischen Argumentationen nicht einig werden. Da Software-Entwicklung Engineering ist, sind oft Vor- und Nachteile gegeneinander abzuwägen. Dabei ist das Gewicht dieser Vor- und Nachteile häufig schwer zu beurteilen, da nicht selten die Ungewissheit der Zukunft eine Rolle spielt. In solchen Fällen sollte man sich dann nicht verzetteln und ggfs. einfach die bereits existierende Lösung mindestens vorläufig akzeptieren, bis mehr Wissen vorhanden ist oder sie mindestens einmal tatsächlich zum Problem wurde. Das gleiche betrifft insbesondere auch Diskussionen über triviale Änderungen.
Fragen: Desweiteren gibt man nicht nur Kritik an den Änderungen, sondern hat immer mal wieder auch einfach nur Fragen zu der Umsetzung, sei es aus Verständnisproblemen oder aus Interesse.
Lob: Wenn jemand wirklich einen guten Job gemacht hat, Lösungen gefunden hat, auf die man vielleicht nie gekommen wäre, dann sollte man auf jeden Fall auch loben.
Dank: Natürlich sollte auch der Umsetzer sich offen für Feedback und letztlich auch dankbar zeigen, um zukünftiges Feedback zu fördern.

Wer macht wann die Code Review?

Während einige Teams in anderen Firmen so vorgehen, dass der Implementierer einen oder mehrere Kandidaten für die Code Review ans Ticket schreiben, gehen wir andere Wege. Da wir davon überzeugt sind, dass jeder Entwickler eines Teams trotz individueller Stärken und Schwächen in der Lage sein sollte, jede Komponente des zu entwickelnden Systems zu bearbeiten, weist der Implementierer keine Code Reviewer zu, da das zu bevorzugten Reviewern führen kann, die dann kaum mehr selbst implementieren. Außerdem könnte sich wieder Wissen über einzelne Komponenten bei wenigen einzelnen Entwicklern sammeln. Stattdessen bekommt ein Ticket den Status “Ready for Review”, wenn es vom Implementierer als fertig befunden wird. Der nächste frei werdende Entwickler zieht sich dieses Ticket dann zur Code Review.

BRICKMAKERS nutzt dazu eine Ticket-Verwaltung, bei dem jedes Ticket einen klaren Status hat. Im Folgenden ist das minimale Set definiert. Jedes Projekt fügt nach Bedarf einige Status hinzu.

Sprint-Backlog: Geplante, noch nicht begonnene Tickets für diesen Sprint.
In Progress: Tickets, die in der Implementierung sind.
Ready for Review: Tickets, die verifiziert werden können.
Review in Progress: Tickets, die im Code Review sind.
Done: Fertige Tickets dieses Sprints.

Die Zustände der Tickets des aktuellen Sprints werden dann auf einem Board in Spalten dargestellt:

Was leistet eine Code Review nicht?

Während Code Reviews insgesamt den Entwicklungsprozess, die Code-Qualität und gewiss auch die Funktionalität der Features sehr positiv beeinflussen, haben sie Grenzen.

Sie können keine Garantie über Funktionalität von Features im weiteren Zeitverlauf geben, da zukünftige Änderungen anderer Features negative Seiteneffekte auf das aktuelle Feature haben können.

Außerdem können sie auch keine Garantie darüber geben, dass durch die Änderungen keine negativen Seiteneffekte auf andere Features verursacht wurden, da meist nur das aktuelle Feature geprüft wird.

Regressionen dieser Art müssen stattdessen mit automatisierten Tests zeitnah erfasst und behoben werden.

Wie lange kann die Code Review dauern?

Das hängt unter anderem stark von den folgenden Faktoren ab und lässt sich daher nicht genau sagen.

Natur des Tickets
Art und Qualität der Commits in der Source Control
Notwendigkeit oder Sinnhaftigkeit von weiteren Änderungen
Je nach Branching-Strategie des Teams auch Merge-Aufwände

Alternative zum Code Review: Pair-Programming

Es gibt Tickets, da macht es mehr Sinn, direkt zusammen daran zu arbeiten. Das könnten sehr komplexe Tickets sein oder etwas, das schwer separat zu verifizieren ist. Auch Aufgaben, die nicht innerhalb eines Tages abgeschlossen werden können, sind potenzielle Kandidaten für Pair-Programming.

Im Fall des Pair-Programmings macht es manchmal keinen Sinn, noch eine dritte Person im Nachgang eine Code Review durchführen zu lassen.

Was passiert mit ungewollten Änderungen?

Nicht selten machen Entwickler aus Eigeninitiative Änderungen, die Features hinzufügen, erweitern oder verändern, die nicht in den Akzeptanzkriterien festgehalten wurden. Auf diese Änderungen hat sich also weder Kunde noch Team committet.

Auch das kann dann bei der Code Review Diskussionen und Entscheidungen auslösen, die besser vor der Umsetzung der Änderungen stattgefunden hätten.

Bei Pair-Programming findet diese Diskussion natürlich sofort noch vor der Umsetzung strittiger Änderungen statt und das kann dann direkt zu einem besseren Ergebnis inklusive Commitment führen.

Code Review als Teil der Fehlerkultur und der Weiterbildung

Code Reviews haben ganz nebenbei auch Einfluss auf die Art, wie wir mit Fehlern umgehen, sowohl bei uns selbst als auch bei anderen.

Im Kern macht man nämlich nichts anderes als sich gemeinsam mit einer anderen Person mit den Fehlern und Verbesserungspotenzial auseinanderzusetzen.

Wenn es alltäglich ist, dass man Dinge hätte besser machen können, wird es normal. Wenn es alltäglich ist, dass andere Dinge hätten besser machen können, wird es normal. Beides gemeinsam erhöht die Akzeptanz einerseits für die eigene Leistung als auch für die der anderen, ohne Stillstand zu fördern.

Zusätzlich kommt es ja auch vor, dass der Reviewer etwas kritisiert, es anspricht und dann den Grund für diese Art der Umsetzung erfährt. In dem Fall lernt also der Reviewer noch dazu.

Außerdem ist man nie alleine für ein Problem verantwortlich, es gehört immer mindestens ein Zweiter dazu. Das Gefühl, dass man selbst an einem Fehler alleine Schuld sei, kann sehr belastend für Entwickler sein.

Die Tatsache, dass Fehler oder Probleme frühzeitig erkannt werden, erlaubt es in den meisten Fällen, schnell mit geringem Aufwand dagegen vorzugehen. Das sorgt natürlich auch für eine höhere Akzeptanz für Fehler, denn die Kritikalität is deutlich geringer im Gegensatz zu Fehlern, die knapp vor Release oder gar erst in Production erkannt werden und dann aufgrund der zeitlichen Entfernung zur Fehlerursache komplett aufwendig analysiert werden müssen.

Das wichtigste ist dabei, dass wir aus Fehlern kontinuierlich dazu lernen.

Fazit

Anforderungen müssen klar genug sein, sodass alle an dem Entwicklungsprozess Beteiligten beurteilen können, wann sie erfüllt sind.
Zur permanenten Sicherstellung hoher Qualität einer Software sollten mindestens zwei Personen die Änderungen gesehen haben und bei Bedarf darüber gesprochen haben.
Das fördert zusätzlich auch die allgemeine Weiterbildung kontinuierlich und beeinflusst positiv die Art und Weise, wie wir mit Fehlern umgehen.
Durch strukturierte Nutzung einer Ticket-Verwaltung und Source Control kann die Qualität der Review erhöht und die Dauer verringert werden.