Von Philippe Schrettenbrunner

Estimated reading time: 6 Minuten

Techblog

Code Reviews – neue Perspektiven auf die Code-Basis

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: Beurteilt wird der Code, nicht der Entwickler. Der Review beruht auf…

Techblog

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!


Über den Autor

Von Philippe Schrettenbrunner

Principal Security Architekt 

Philippe Schrettenbrunner, stellvertretender Bereichsleiter Cybersecurity, ist seit 2005 bei MaibornWolff. Als Techie durch und durch, bildet für ihn Qualität die Grundlage des Handelns. Sein Fokus liegt hierbei auf dem Backend. Philippe ist Diplom-Informatiker und hat zudem einen Master of Engineering in Computer Science. In seiner Freizeit ist er Portraitfotograf. 

LinkedIn: https://www.linkedin.com/in/philippe-schrettenbrunner-5a8a87225/