45 lines
5.8 KiB
TeX
45 lines
5.8 KiB
TeX
\section{Code-Reviews}
|
|
\label{sec:code-reviews}
|
|
|
|
\subsection{Umfang}
|
|
|
|
Die Umsetzung verteilte sich auf elf Pull Requests mit insgesamt 113 Diskussionssträngen. Tabelle~\ref{tab:pull-requests} im Anhang enthält die vollständige Übersicht mit Laufzeiten, Reviewern und Strangzahl. Kein Pull Request wurde abgelehnt oder verworfen; sämtliche abgeschlossenen erhielten eine Freigabe. Die inhaltliche Auseinandersetzung fand also durchgängig in den Diskussionssträngen statt und nicht über Ablehnungen.
|
|
|
|
Dass kein einziger Pull Request verworfen werden musste, ist kein Zufall. Die vorgelagerte Klärung — die Feature-Analyse im Juni und die Backlog-Durchsicht im Juli — hatte die fachlichen Fragen so weit beantwortet, dass keine Implementierung auf einer falschen Annahme beruhte. Der Aufwand der Vorklärung zahlte sich damit unmittelbar aus.
|
|
|
|
\subsection{Prüftiefe und Risiko}
|
|
|
|
Bemerkenswert ist die Verteilung der Diskussionsstränge über die Pull Requests. Sie ist stark ungleich, folgt aber erkennbar dem Risiko der jeweiligen Änderung.
|
|
|
|
Die intensivste Prüfung erfuhren der Einzeldownload mit 30 und die Freigabelinks mit 23 Diskussionssträngen — also genau die beiden Arbeiten, bei denen ein Fehler zur Offenlegung von Kundendokumenten geführt hätte. Der Document Explorer als Grundlage des Moduls folgt mit 21 Strängen. Am unteren Ende steht die PDF-Vorschau mit zwei Strängen: eine reine Darstellungsfunktion, die auf bereits geprüften Bausteinen aufsetzt und keine eigene Sicherheitsentscheidung trifft.
|
|
|
|
Diese Verteilung entstand ohne ausdrückliche Vorgabe. Sie deutet darauf hin, dass die Reviewer ihre Aufmerksamkeit intuitiv dorthin lenkten, wo ein Fehler teuer gewesen wäre — ein Verhalten, das sich mit einer formalen Vorgabe kaum hätte erzwingen lassen.
|
|
|
|
\subsection{Wiederkehrende Themen}
|
|
|
|
Über alle Diskussionsstränge hinweg lassen sich fünf Muster erkennen.
|
|
|
|
\textbf{Sicherheit und Eingabeprüfung.} Der größte Anteil entfiel auf die Frage, ob eine von außen kommende Angabe ausreichend geprüft wird. Konkret betraf das die Pfadprüfung beim Download, den Verzicht auf das Durchreichen von Dateiströmen zugunsten zeitlich begrenzter Zugriffs-URLs, den bewussten Ausschluss benutzerdefinierter Symbole aus der Vorschau und den Grundsatz, keine Dateiinhalte über nicht authentifizierte Pfade auszuliefern.
|
|
|
|
\textbf{Kompatibilität vor Ideallösung.} Mehrfach wurde eine technisch sauberere Lösung zugunsten einer verlässlich funktionierenden verworfen. Das prägnanteste Beispiel ist die Bereitstellung der Vorschaubilder als Rastergrafik, weil die Vektorvariante von verbreiteten Messengern nicht zuverlässig dargestellt wird (siehe Abschnitt~\ref{sec:pdf-preview}).
|
|
|
|
\textbf{Abgrenzung statt Ausweitung.} Im Review erkannte Probleme wurden konsequent als neue Backlog Items erfasst, statt sie im laufenden Pull Request mitzuerledigen. Das betrifft die Speicherabfrage aus dem Explorer und der Suche, die Typsuche und die Nebenläufigkeit im Lookup. Dieses Vorgehen hielt die Pull Requests auf ihren jeweiligen Gegenstand begrenzt und machte zugleich sichtbar, dass die Probleme erkannt und nicht übergangen wurden.
|
|
|
|
\textbf{Struktur und Wartbarkeit.} Wiederkehrend waren Hinweise auf auszulagernde Skripte in Seitenvorlagen, auf fehlertolerantes statt manuelles Auswerten von Aufzählungswerten, auf überflüssige Kommentare und auf uneinheitliche Benennungen.
|
|
|
|
\textbf{Fachliche Klärung im Review.} In mehreren Fällen führte die Diskussion nicht zu einer Änderung am Code, sondern am Backlog Item. So wurde die Antwort bei fehlender Berechtigung von 404 auf 403 geändert und festgehalten, dass auch ein einzeln ausgewähltes Dokument als Archiv ausgeliefert wird. Das Review leistete damit auch Anforderungsarbeit — ein Hinweis darauf, dass sich Akzeptanzkriterien im Vorfeld nie vollständig formulieren lassen.
|
|
|
|
\subsection{Wechsel der Reviewer}
|
|
|
|
Über die Projektlaufzeit wechselte der Hauptreviewer zweimal: \emph{Timo Walter} prüfte die fünf Pull Requests der Anfangsphase, \emph{Sarah Hinzmann} übernahm Anfang August, \emph{Robin Noack} die Schlussphase. Zusätzlich beteiligte sich \emph{Hanna Ebner} an den frühen Diskussionen.
|
|
|
|
Der Wechsel war organisatorisch bedingt, hatte aber einen erkennbaren fachlichen Effekt. Die Schwerpunkte verschoben sich mit den Personen: Die frühen Reviews befassten sich stark mit Architektur, Sicherheit und der Grundsatzfrage der Speicherabfrage; die mittleren mit Bedienung und Wartbarkeit; die späten mit Benennung, Fehlertoleranz und Codestil. Ein durchgehend gleicher Reviewer hätte diese Bandbreite vermutlich nicht abgedeckt, weil sich Aufmerksamkeitsmuster mit der Zeit verfestigen. Zugleich verteilte der Wechsel das Wissen über das neue Modul im Team.
|
|
|
|
\subsection{Kritische Betrachtung des Vorgehens}
|
|
|
|
Ein Punkt verdient eine selbstkritische Bewertung. Am 27.~Juli wurden vier Pull Requests am selben Tag eröffnet, ein fünfter folgte am Tag darauf. Da sie inhaltlich aufeinander aufbauten, ließ sich nur der erste zeitnah abschließen; die übrigen blieben zwischen zwei und drei Wochen offen und wurden erst nach dem Zusammenführen ihrer jeweiligen Vorgänger fertiggestellt.
|
|
|
|
Die in Abschnitt~\ref{sec:architecture} beschriebene Verkettung machte die einzelnen Änderungen zwar gut prüfbar, verlagerte den Aufwand aber an das Ende: Der überwiegende Teil der Zusammenführungen fällt in die Woche vom 18. bis 21.~August — unmittelbar vor Abnahme und Produktivsetzung. Diese Ballung erhöhte den Druck in einer ohnehin kritischen Phase.
|
|
|
|
Rückblickend wäre ein stärker sequenzielles Vorgehen vorzuziehen gewesen: jeweils einen Pull Request abschließen, bevor der nächste eröffnet wird. Der Vorteil paralleler Bearbeitung — nie auf ein Review warten zu müssen — erwies sich als geringer als erwartet, weil die inhaltliche Abhängigkeit ein echtes paralleles Vorankommen ohnehin verhinderte.
|