Nachvollziehbares Code Review mit Doxygen und GitLab

Intro

In dem einen oder anderen Blog-Artikel haben wir uns bereits mit Code-Reviews auseinandergesetzt, Wozu also noch ein Artikel zu dem Thema?
Nun: Hier geht es um einen Prozess, der sich bei uns bei der Durchführung von Code-Reviews bewährt hat. Andere Artikel zum Thema Code Review finden Sie hier (2015) und hier (2017).

Warum?

Im Laufe eines Projektes fiel auf, dass das Review eines Moduls bereits erfolgt war. Ein entsprechender Vermerk in einer extra zu diesem Zweck geführten Tabelle war versehentlich geändert worden – mit dem Effekt, dass das Modul nochmal gereviewed wurde (durch später hinzugekommene Projektmitarbeiter, die das natürlich nicht wissen konnten). So etwas passiert unweigerlich dann einmal, wenn der Status eines Items losgelöst vom Item selbst getrackt wird.
Obendrein ist das manuelle Auflisten der Module mit Findings oder noch zu reviewender Module fehlerträchtig und zeitraubend (und eine obendrein langweilige Tätigkeit). Das muss doch besser gehen, ohne dass man jetzt gleich auf möglicherweise teure Tools zurückgreifen muss!

Wohin?

Die Ziele des neuen Prozesses waren schnell definiert:

• „Single-Source“ – d.h. keine separaten, vom Code losgelösten Dokumente
• Einfaches Tracking des Review-Status von Source-Modulen
• Nachverfolgbarkeit von Reviews
• Möglichst wenige weitere Tools

Womit

Bei MEDtech führen wir Code Reviews mittels Doxygen-Kommentaren direkt im Source-Code durch. GitLab unterstützt dabei das Branching/Merging während des Reviews und die Rückverfolgbarkeit genauso wie die Entwicklung selbst. Will heißen: Git-Abläufe und Doxygen sind in den Projekten bereits etabliert.
Netterweise generiert Doxygen mit Hilfe des Tags „@xrefitem“ selbst definierbare Listen, z.B. im HTML-Format. Dadurch können wir aus dem Source-Code Listen der Module mit ihrem jeweiligen Review-Status (siehe unten) erzeugen. Diese lassen sich z.B. in das Meilenstein-Review-Dokument kopieren (wobei sich dieser Schritt auch noch automatisieren ließe 😉).

Vorbereitung

Um Tippfehler und Schreibarbeit bei den @xrefitem-Kommentaren zu vermeiden, werden in der Doxygen Konfigurationsdatei folgende Aliase definiert:

ALIASES += "review_done=\xrefitem reviews_done \"Code review done\" \"Code Review: Modules done\""
ALIASES += "review_missing=\xrefitem reviews_missing \"Code review missing\" \"Code Review: Modules with without reviews\""
ALIASES += "review_finding=\xrefitem review_findings \"Code review finding\" \"Code Review: Modules with findings\""

Die Tags @review_missing, @review_finding, @review_done werden wie andere Doxygen-Tags verwendet, sollten jedoch im File-Header-Kommentar gesetzt werden. Der Text hinter dem Tag erscheint dann im generierten Dokument (z.B. HTML) unterhalb des Dateinamens:

Mit dieser Konfiguration erstellt Doxygen Einträge in den Listen „Code review: Modules done“, „Code review: Modules missing“ und „Code review: Modules with findings“. Das sollte idealerweise auf Knopfdruck möglich sein (man denke an die Projektleitung) oder zumindest in der CI-Pipeline erfolgen.
Als Schmankerl bekommt das Management auf einfachste Weise noch ein KPI und das Development-Team ein Fleißbildchen 😉

Das Ziel ist also definiert – nun zum Weg dorthin.

Der Prozess

Im Folgenden wollen wir ein Code-Review für ein fiktives Software-Modul durchspielen (wie in der Grafik unten gezeigt). Das Review-Item (=Code) wird im Laufe des Prozesses verschiedene Status durchlaufen, die sich durch die Aktionen der beteiligten Personen „Developer“ und „Reviewer“ ändern. Die Status sind:

• review missing
• review findings
• review done

Start

Eines Vorweg: wir haben eine Grafik erstellt, welche die nachfolgenden Schritte visualisiert. Die Grafik finden Sie am Ende des Blogbeitrags.
Der Code-Review Prozess beginnt mit der Neu-Erstellung bzw. Überarbeitung unseres Software-Moduls im Status „review missing“. Der Developer erstellt dazu ein Review-Ticket (im Folgenden als Issue bezeichnet) in GitLab und weist das dem Reviewer zu. Dabei wird auch ein eigener Git-Branch inkl. Merge Request erstellt (alternativ kann das Review auch direkt auf dem Feature-Entwicklungsbranch erfolgen). Für den Merge Request sollte der WiP („work in progress“) status benutzt werden, damit niemand den Merge Request versehentlich schließen kann. In GitLab erledigt man das alles mit einem Klick („Create branch and merge request“) innerhalb des jeweiligen Issues.

Review

Nun beginnt der Reviewer seine Arbeit, indem er zunächst im Code-Header folgendes Template einfügt (der Übersichtlichkeit halber hier ein reduziertes Template-Beispiel). Es enthält das Status-Alias zusammen mit dem Namenskürzel des letzten Committers, dem Commit-Datum und zum Schluss den Commit-Hash.
Anmerkung: Da Doxygen die Zeilenumbrüche innerhalb eines Kommentarblocks entfernt, sind zwecks besserer Lesbarkeit im Ausgabeformat die manuellen Zeilenumbrüche „@n“ erforderlich.

/**
* @file exempleUnit.c
* @brief example for code review demonstration
*
* @review_finding &ltREVIEWER&gt, &ltDATE&gt, &ltHASH&gt
* @n 1. Completeness of implementation
* @n 2. General code quality
* @n - 2.1 Nullpointer check:
* @n - 2.2 No infinite loops:
* @n - 2.3 No open Todos in code:

In der Kommentarstruktur werden die entsprechenden Findings dokumentiert, z.B.:

* @review_findings RVW, 03.05.2022, &ltHASH&gt
* @n 1. Completeness of implementation: ok
* @n 2. General code quality
* @n - 2.1 Nullpointer check:
* @n RVW finding: Missing Check in foo()
* @n - 2.2 No infinite loops: ok
* @n - 2.3 No open Todos in code: ok

Anmerkung: Die Änderungen am Review-Tag sind in Fettdruck hervorgehoben.
Im unwahrscheinlichen Fall, dass es beim Review keine findings gibt, ändert der Reviewer den Status zu „review done“ (fertig sind wir aber noch nicht). Normalerweise gibt es nach dem ersten Review – wie in diesem Fall – allerdings findings. Dadurch ändert der Reviewer den Status zu „review finding“. Nun committed er die Änderungen und weist das Issue dem Developer zu.

Bearbeitung

Der Developer bearbeitet die Findings. Er kann ein Finding …

• … zurückweisen (reject): in diesem Falle sind sich Developer und Reviewer uneins. Es sollte diskutiert werden, ob hier ein fix vorgenommen werden muss oder nicht.
• … reparieren (fix): der Developer stimmt dem Reviewer zu, dass hier nachgearbeitet werden soll und behebt das finding.
• … akzeptieren (accept): der Developer stimmt zwar dem Reviewer zu, dass nachgearbeitet werden sollte, kann das finding aber nicht beheben (z.B. aus Zeitgründen).

Die Aktionen werden im Review-Tag festgehalten:

* @review_findings RVW, 03.05.2022, &ltHASH&gt
* @n 1. Completeness of implementation: ok
* @n 2. General code quality
* @review_findings RVW, 03.05.2022, &ltHASH&gt
* @n - 2.1 Nullpointer check:
* @n RVW finding: Missing Check in foo()
* @n DVP fix: used semaphore for function foo().
* @n - 2.2 No infinite loops: ok
* @n - 2.3 No open Todos in code: ok

Das Ganze wird daraufhin wieder eingecheckt. Jetzt weist der Developer das Issue erneut dem Reviewer zu.
Hinweis: Der Review-Status wird mit der Überarbeitung durch den Developer nicht geändert!

Re-Review

Die Fixes und Rejects werden in diesem Review-Schritt überprüft und entsprechend kommentiert. Ist der Reviewer mit den Änderungen und/oder den Begründungen der Rejects durch den Developer zufrieden, so bestätigt er dies durch ein einfaches „ok“ im Review-Tag. Diese Kommentar-Änderung wird wieder eingecheckt – denn für das Abschließen des Reviews im nächsten Schritt benötigen wir ja die dazugehörige Revision-Nummer (=Hash) für die erforderliche Nachverfolgbarkeit. Und letztlich soll im Release-Dokument auf die Version verwiesen werden, die den vollständigen Review-Kommentar enthält.

* @review_findings RVW, 03.05.2022, &ltHASH&gt
* @n 1. Completeness of implementation: ok
* @n 2. General code quality
* @n - 2.1 Nullpointer check:
* @n RVW finding: Missing Check in foo()
* @n DVP 2022-06-03: fixed: used semaphore for function foo().
* @n RVW 2022-06-04: confirmed
* @n - 2.2 No infinite loops: ok
* @n - 2.3 No open Todos in code: ok

Bestehen weiterhin Zweifel an der Implementierung bzw. der Entscheidung des Developers, so geht es in eine weitere Überarbeitungsrunde.

Abschluss

Nehmen wir mal an, dass sich Reviewer und Developer einig sind und das Review beendet werden kann. In unserem Beispiel ändern wir schließlich den Review-Status zu „@review_done“, tragen den Git-Hash vom vorausgegangenen Commit ein und räumen die jetzt überflüssig gewordenen Finding-Kommentare auf:

/**
* @file exempleUnit.c
* @brief example for code review demonstration
*
* @review_done RVW, 03.05.2022, 002e9d43bde307687aee6bd8bc3075838645290e
*/

Bleiben Findings bestehen, ändert sich der Status „@review_finding“ auch nach Beenden des Reviews nicht.
Nach dem Commit entfernt der Reviewer den WiP-Status des MRs (=Ändern des MR-Titels) und weist den Merge-Request dem Developer zu, welcher den Merge durchführt. Der erlösende Klick auf den Merge-Button innerhalb des Mergerequests in GitLab schließt nach erfolgreichem Merge des zugehörigen Branches auf den Master-Branch zudem automatisch das zugehörige Issue und den Merge-Request selbst. Das Code-Review unseres Moduls ist damit beendet.

Resümee

Wir haben in diesem Blogartikel gezeigt, dass nachvollziehbare Code-Reviews mit den anfangs gesteckten Zielen und den Tools GitLab und doxygen machbar und praktikabel sind. Natürlich interessiert uns, was Sie davon halten oder – noch besser – ob Sie selbst „schlichtere“ Prozesse etabliert haben, denn komplizierter geht bekanntlich immer und in der Kürze liegt die Würze

Falls Sie also Anmerkungen, Anregungen oder Fragen haben, zögern Sie nicht uns zu kontaktieren. Hierfür können Sie gerne die Kommentarfunktion benutzen oder uns direkt über E-Mail (info@medtech-ingenieur.de) oder Telefon (09131/691240) kontaktieren.