Techblog

Code Reviews – neue Perspektiven auf die Code-Basis

Von Philippe Schrettenbrunner
16. Februar 2017

Meine sicher-noch-nicht-vollständige-und-trotzdem-absolut-teilenswerte Checkliste für Code Reviews.

Code Reviews verbessern die Softwarequalität und verteilen Wissen über die Software im Team. Je mehr Entwickler den Code der Kollegen anschauen, desto besser erreichen wir beide Ziele. Für effiziente und gewinnbringende Code Reviews sind mir drei Punkte wichtig:

  1. Beurteilt wird der Code, nicht der Entwickler.
  2. Der Review beruht auf Projektstandards und -richtlinien.
  3. Das ganze Team übernimmt Verantwortung für die Code-Qualität.

In meinen letzten Projekten wählten wir den Reviewer oft nach dem Zufallsprinzip. Das bedeutet natürlich auch, dass junge Entwickler regelmäßig Code von erfahrenen Kollegen reviewen. Das Vorgehen hat aus mehreren Gründen Vorteile:

  • Der Reviewer muss sich intensiv mit dem Code beschäftigen und kennt sich anschließend ähnlich gut aus wie derjenige, der den Code geschrieben hat. Durch das geteilte Wissen ist das Projekt weniger abhängig von einzelnen Entwicklern, die schließlich auch mal Urlaub nehmen wollen.
  • Jüngere Kollegen lernen besser programmieren, wenn sie die Arbeit der anderen analysieren: Sie sehen, wie ein anderer, möglicherweise erfahrener Kollege Patterns einsetzt oder Programmierparadigmen umsetzt.
  • Durch den Review sieht jeder Entwickler, was seine Kollegen machen. Auf diese Weise hat das gesamte Team einen besseren Überblick über die tatsächliche Gesamtarchitektur und versteht Zusammenhänge und Auswirkungen besser.
  • Vier Augen sehen immer mehr als zwei. Und ein frisches Paar Augen sieht oft am meisten: Jüngere Kollegen hinterfragen meine Lösung. Sie wollen verstehen, warum ich etwas „So“ programmiert habe – und nicht „So“. Ihre Unvoreingenommenheit hilft mir zu reflektieren, ob eine andere Lösung ebenso konform mit den Programmierprinzipien ist. Oder sogar besser passt. In jedem Fall lernt der Kollege etwas dazu. Und ich vielleicht auch.

Ein Code Review darf aber nicht willkürlich sein. Er muss sich an den Projektstandards und Entwicklungsrichtlinien orientieren. Gerade jüngere Kollegen stellen mir oft die Frage, ob es für den Review nicht eine Checkliste gibt. Deswegen habe ich angefangen, meine Best Practices zu sammeln und im Projekt zur Verfügung zu stellen. Einige Kriterien sind projektspezifisch, aber viele sind allgemeingültig. Meine Checkliste hat vier Kategorien: 

  • High-Level-Architektur und -Design, 
  • Funktionalität, 
  • Lesbarkeit und Wartbarkeit, 
  • nicht-funktionale Anforderungen.

Ich erhebe mit ihr keinen Anspruch auf Vollständigkeit – im Gegenteil: Die Liste wächst immer noch, und von Projekt zu Projekt sind jeweils andere Aspekte wichtig. 

High-Level-Architektur und -Design

  • Passt der Code in die Gesamtarchitektur des Projektes?
  • Wurden alle Projektvorgaben, zum Beispiel Namenskonventionen oder Style Guide, eingehalten?
  • Wurden neue Design Patterns oder Datenstrukturen eingeführt und sind diese passend?
  • Ist der Code an der richtigen Stelle (Systeme, Komponenten, Schichten...)?
  • Sind technischer und fachlicher Code sauber getrennt?
  • Ist zusammenstehender Code (in einer Methode, in einer Klasse) auch auf dem gleichen Abstraktionsniveau? Oder sind Highlevel-Konzepte mit Lowlevel-Details vermischt?
  • Wurde etwas entwickelt, das schon vorhanden war? Oder etwas, das besser durch eine Bibliothek abgedeckt werden sollte?
  • Wurden neue Abhängigkeiten erzeugt, die unnötig sind (zum Beispiel zu einer zweiten JSON Bibliothek)?

Funktionalität

  • Tut der Code das, was im Ticket gefordert ist?
  • Macht er mehr, als im Ticket gefordert?
  • Wurde der Code überzüchtet, neu-deutsch „over-engineered“?
  • Macht der Code gefährliche Annahmen, etwa über Defaults, Wertebereiche oder Verfügbarkeit von Systemen/Informationen?
  • Gibt es subtile Bugs, etwa ein < statt <=? Andere Beispiele sind die falsche Zuweisung von Feldern oder fehlende, aber notwendige Klammern bei Bool‘schen Ausdrücken.
  • Sind die Akzeptanzkriterien als Test umgesetzt?

Lesbarkeit und Wartbarkeit

  • Lässt sich einfach erkennen, was der Code tut? Sind auch die Tests selbsterklärend?
  • Spiegeln die verwendeten Namen (Felder, Variablen, Parameter, Methoden, Klassen...) tatsächlich das wider, wofür sie stehen?
  • Sind komplexe Codestellen und ungewöhnliche Sonderfälle entsprechend kommentiert?
  • Gibt es überraschende Seiteneffekte?
  • Decken die Tests die Kernfunktionalität ab. Und zwar für Gut- und Fehlerfälle?
  • Liefern Fehlermeldungen/Logs genügend Kontextinformationen, um später das Verhalten der Software nachzuvollziehen?

Nichtfunktionale Anforderungen

  • Gibt es mögliche Sicherheitsprobleme mit dem Code? Zum Beispiel ungeprüfte Eingabewerte oder durch die Verletzung von OWASP-Regeln.
  • Geht der Code verschwenderisch mit Systemressourcen um? Dazu gehören unnötige Datenbank-Verbindungen, viele Aufrufe externer APIs oder Berechnungen, die mehrfach gemacht werden.
  • Sind Texte für Endnutzer auf Tipp- und Grammatikfehler geprüft?

 

Selbstverständlich kann ein Reviewer Code nach diesen Leitfragen nur analysieren, wenn der Entwickler sauberen Code abliefert. Mindestanforderung dafür ist bei uns: Der Code ist richtig formatiert und kommentiert. Alle Tests laufen durch. Die statische Code Analyse meldet keine offensichtlichen Fehler. und es gibt keine TODOs, FIXMEs oder Debug Statements mehr im Code. Mit anderen Worten: Aus Sicht des Entwicklers ist sein Ticket abgeschlossen.

Fehlt Ihnen etwas? Ich freue mich auf Ihre Anregung im Kommentarfeld unten!

Kommentare

Sehr guter Blog-Eintrag, mit vielen interessanten Aspekten. Insbesondere der Punkt, dass der Code und nicht der Entwickler beurteilt wird halte ich für ein grundlegend wichtiges Mindset. Gerät dies in Vergessenheit, kann es schnell demotivierend auf den Entwickler wirken oder Reviews werden aus Angst/Rücksichtnahme nicht mehr korrekt durchgeführt. Dies ist natürlich bei einem gut eingespielten Team weniger relevant, als bei einem ganz neuen oder sehr heterogenem Team bzw. Teammitglieder.

Spannend wäre noch die Tooling-Frage. Meiner bisherigen Erfahrung nach sind entsprechende Tools meistens teuer und bieten geringen Mehrwert, sodass es doch auf die Commit-Historie zusammen mit der üblichen IDE hinausläuft. Hat hier jemand bessere Erfahrungen gemacht?

Vielen Dank. Auf die Tool-Frage bin ich bewusst nicht eingegangen, denn diese ist oft von vielen externen Faktoren abhängig.
 
Für einen Code Review ist eigentlich kein Tool nötig. In einem früheren Projekt haben wir uns einfach eigene todo Tags in der IDE angelegt (REV und REVCRIT) und Anmerkungen direkt im Code gemacht. Der TODO Tab der IDE diente somit als Übersicht über alle noch offenen Review Anmerkungen. 

Da sich bei uns Gitlab als Webfrontend für die Versionsverwaltung etabliert hat, nutze ich in vielen Projekten die dort eingebauten Code Review Möglichkeiten und bin sehr zufrieden. 

Abhängig von der Projektgröße kann es natürlich sinnvoll sein, mächtigere Tools mit entsprechenden Workflows einzusetzen. Gerade, wenn ein Vier- bzw. Sechsaugenprinzip erzwungen werden soll. Wichtig ist, dass der Prozess Code Reviews unterstützt und fördert und nicht die Weiterentwicklung hemmt. 

Neuen Kommentar schreiben

Public Comment form

  • Zulässige HTML-Tags: <a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> <dl> <dt> <dd><p><h1><h2><h3>

Plain text

  • Keine HTML-Tags erlaubt.
  • Internet- und E-Mail-Adressen werden automatisch umgewandelt.
  • HTML - Zeilenumbrüche und Absätze werden automatisch erzeugt.

ME Landing Page Question

Erhalten Sie regelmäßig Praxis-Tipps per E-Mail

Praxis-Tipps per E-Mail erhalten