Code Review Checklist: What I Look for After 10 Years of PRs \u2014 TXT1.ai

Vor drei Jahren genehmigte ich einen Pull-Request, der meinem Unternehmen am vergangenen Wochenende 47.000 USD an entgangenem Umsatz kostete. Der Code sah gut aus. Die Tests bestanden. Die Logik war solide. Aber ich übersah etwas Subtiles in der Art und Weise, wie wir Zeitkonversionen für unsere europäischen Kunden handhabten, und als die Sommerzeit begann, begann unser Zahlungsabwicklungssystem, heimlich für 18 % unserer Benutzerbasis auszufallen.

Dieser Vorfall hat für immer verändert, wie ich Code überprüfe. Ich bin Sarah Chen und ich war in den letzten zehn Jahren Senior Engineering Managerin in drei verschiedenen SaaS-Unternehmen und habe durchschnittlich 15-20 Pull-Requests pro Woche überprüft. Das sind ungefähr 8.000 PRs in meiner Karriere, von einzeiligen Hotfixes bis hin zu massiven architektonischen Umgestaltungen, die über 50 Dateien reichen. Ich habe brillanten Code gesehen, der Bugs ausgeliefert hat, und chaotischen Code, der jahrelang fehlerfrei in der Produktion lief.

Was ich gelernt habe, ist, dass die Codeüberprüfung nicht wirklich darum geht, Bugs zu finden—obwohl das wichtig ist. Es geht darum, Systeme zu schaffen, die den Kontakt mit der Realität überstehen. Es geht darum, Code zu erstellen, den der nächste Entwickler (oft du selbst, sechs Monate später) verstehen und ohne Angst ändern kann. Und es geht darum, die unsichtbaren Probleme zu erkennen, die sich erst zeigen, wenn deine Anwendung skaliert, wenn Randfälle auftauchen oder wenn dieser eine Kunde in Australien etwas tut, das du niemals vorhergesehen hast.

Dies ist meine Checkliste. Nicht die theoretische aus Lehrbüchern, sondern die kampferprobte, die ich durch unzählige Produktionsvorfälle, nächtliche Debugging-Sitzungen und dieses nagende Gefühl, wenn du realisierst, dass ein Bug, den du genehmigt hast, nun tatsächliche Nutzer betrifft, verfeinert habe.

Die ersten 30 Sekunden: Was die PR-Beschreibung mir sagt

Bevor ich auch nur eine Zeile Code anschaue, verbringe ich 30 Sekunden damit, die PR-Beschreibung zu lesen. Das mag trivial erscheinen, aber eine schlecht geschriebene Beschreibung ist oft das erste Warnsignal, dass der Code selbst Probleme haben könnte. Aus meiner Erfahrung können Entwickler, die nicht klar kommunizieren können, was ihr Code tut, oft nicht vollständig über die Implementierung nachgedacht haben.

Ich suche nach drei spezifischen Dingen: das zu lösende Problem, den gewählten Ansatz und eventuell getroffene Abwägungen. Eine gute PR-Beschreibung liest sich wie ein Mini-Design-Dokument. "Bug in der Benutzerauthentifizierung behoben" sagt mir nichts. "Rennbedingungen im JWT-Token-Refresh behoben, die dazu führten, dass 0,3 % der Nutzer während stark frequentierter Zeiten unerwartet abgemeldet wurden, indem ein verteiltes Lock mit Redis implementiert wurde" zeigt mir, dass der Entwickler das Problem tiefgreifend versteht.

Ich überprüfe auch, ob die PR angemessen dimensioniert ist. Mein allgemeines Daumenmaß: Wenn eine PR mehr als 400 Codezeilen berührt (ohne Tests und generierte Dateien), ist sie wahrscheinlich zu groß, um sie effektiv zu überprüfen. Forschungen aus den Ingenieurtätigkeiten von Google legen nahe, dass die Effektivität der Überprüfung dramatisch abfällt, nachdem 200-400 Zeilen erreicht wurden. Ich habe festgestellt, dass das zutrifft—nach der Überprüfung von etwa 300 Zeilen beginnt meine Aufmerksamkeit nachzulassen, und ich beginne, subtile Probleme zu übersehen.

Wenn ich auf eine massive PR stoße, bitte ich den Entwickler, sie in kleinere, logische Teile zu zerlegen. Ja, das kostet anfangs mehr Zeit, aber es verhindert das Szenario, in dem ich einen 2.000-Zeilen-PR abnicke, weil ich überfordert bin und ihn einfach hinter mich bringen möchte. Ich habe auf diese Weise zu viele problematische PRs in meiner frühen Karriere genehmigt.

Die Beschreibung sollte auch auf relevante Kontexte verlinken: das Ticket oder Problem, das angesprochen wird, alle Entwurfsdokumente und verwandte PRs. Wenn ich in Jira oder Slack herumsuchen muss, um zu verstehen, warum diese Änderung vorgenommen wird, ist das Reibung, die den gesamten Überprüfungsprozess verlangsamt. In meinem aktuellen Unternehmen haben wir eine Regel aufgestellt, dass PRs ohne den richtigen Kontext nicht überprüft werden, bis die Beschreibung aktualisiert wird. Diese einfache Regel hat die Qualität unserer Überprüfungen messbar verbessert.

Die Logikschicht: Löst das tatsächlich das Problem?

Wenn ich verstehe, was die PR tun soll, verifiziere ich, dass sie es tatsächlich tut. Das klingt offensichtlich, aber du würdest überrascht sein, wie oft Code Tests besteht, aber das zugrunde liegende Problem nicht vollständig angeht. Ich habe Entwickler gesehen, die das Symptom beheben, während die Ursache unbeachtet bleibt, und eine Zeitbombe schaffen, die später explodiert.

"Codeüberprüfung geht nicht wirklich darum, Bugs zu finden—es geht darum, Systeme zu bauen, die den Kontakt mit der Realität überstehen und Code zu erstellen, den der nächste Entwickler ohne Angst verstehen kann."

Ich beginne, indem ich gedanklich den glücklichen Pfad durchlaufe. Wenn dies eine neue Funktion für die Verarbeitung von Rückerstattungen ist, stelle ich mir vor, dass ein Kunde eine Rückerstattung anfordert, und verfolge den Code-Pfad vom API-Endpunkt durch die Geschäftslogik bis zur Datenbankaktualisierung. Macht jeder Schritt Sinn? Behandeln wir die Daten korrekt? Aktualisieren wir alle erforderlichen Datensätze?

Aber der glückliche Pfad ist einfach. Was guten Code von produktionsbereitem Code unterscheidet, ist, wie er mit den unglücklichen Pfaden umgeht. Was passiert, wenn das Zahlungsgateway ausfällt? Was, wenn der Rückerstattungsbetrag negativ ist? Was, wenn der Benutzer eine Rückerstattung für eine bereits zurückerstattete Bestellung anfordert? Ich habe gelernt, in Bezug auf Randfälle fast paranoid zu sein, denn Produktionsumgebungen sind Chaosmaschinen, die jeden Randfall finden, den du nicht in Betracht gezogen hast.

Ich achte auf defensive Programmiermuster: Null-Prüfungen, Eingangsvalidierung, Behandlung von Grenzbedingungen. Aber ich achte auch auf Überengineering. Nicht jede Funktion muss jeden möglichen Randfall behandeln. Wenn eine private Methode nur an einem Ort mit validierten Eingaben aufgerufen wird, ist es einfach nur Lärm, redundante Validierungen hinzuzufügen. Der Schlüssel ist, den Vertrag jeder Funktion zu verstehen und sicherzustellen, dass er an den Grenzen eingehalten wird.

Ein Muster, das ich wiederholt Probleme verursachen sah, ist das "optimistische Update." Ein Entwickler geht davon aus, dass ein Vorgang erfolgreich sein wird, und aktualisiert den UI- oder Datenbankzustand, bevor der Erfolg bestätigt ist. Das funktioniert 99 % der Zeit, aber dieses 1 % führt zu unglaublich verwirrenden Bugs, bei denen der Systemzustand inkonsistent wird. Ich kennzeichne immer diese Muster und frage: Was passiert, wenn dieser Vorgang mitten drin fehlschlägt?

Die Datenschicht: Den Informationsfluss verfolgen

Datenfehler sind die schlimmsten Arten von Fehlern, weil sie oft still und kumulativ sind. Ein Logikfehler könnte deine Anwendung sofort zum Absturz bringen, aber ein Datenfehler könnte deine Datenbank über Wochen hinweg langsam beschädigen, und bis du es bemerkst, hast du Tausende von fehlerhaften Datensätzen und keinen sauberen Weg, um sie zu reparieren.

Bei der Überprüfung von Code, der Daten berührt, verfolge ich den gesamten Lebenszyklus dieser Daten. Woher kommen sie? Wie werden sie validiert? Wie werden sie transformiert? Wo werden sie gespeichert? Wer kann darauf zugreifen? Dies ist besonders wichtig für benutzergenerierte Inhalte oder Finanzdaten, bei denen Fehler reale Folgen haben.

Besonders achte ich auf Datentypkonversionen. Ich habe einmal Code genehmigt, der einen Preis von einem Float in einen Integer für eine Berechnung konvertierte und dann zurück zu einem Float. Schien harmlos. Aber wegen Problemen mit der Gleitpunktgenauigkeit konnten Preise wie 10,99 USD gelegentlich zu 10,98 USD oder 11,00 USD nach der Rückreise werden. Wir haben es nicht bemerkt, bis Kunden über falsche Gebühren zu klagen begannen. Jetzt überprüfe ich jede Typkonversion, insbesondere bei Geld- oder Maßangaben.

Datenbankmigrationen sind ein weiterer Bereich, in dem ich besonders vorsichtig bin. Ich achte auf mehrere Dinge: Ist die Migration rückgängig zu machen? Was passiert, wenn sie mitten drin fehlschlägt? Wie lange wird es auf unserer Produktionsdatenbank dauern? Ich habe Migrationen gesehen, die auf einer Entwicklungsdatenbank mit 1.000 Datensätzen gut funktionierten, aber Tabellen für 20 Minuten auf einer Produktionsdatenbank mit 50 Millionen Datensätzen sperrten, wodurch die gesamte Anwendung ausfiel.

Für alle Codes, die bestehende Daten ändern, frage ich: Gibt es einen Backup-Plan? Können wir das zurücksetzen, wenn etwas schiefgeht? In meinem vorherigen Unternehmen hatten wir eine Regel, dass jede Datenmigration, die mehr als 10.000 Datensätze betrifft, einen Trockenlauf auf einem Produktionssnaphot und ein explizites Rollback-Verfahren erfordert, das im PR dokumentiert ist. Das hat uns mehrfach gerettet.

Ich achte auch auf Datenzugriffsmuster. Führt dieser Code N+1-Abfragen aus? Lädt er gesamte Sammlungen in den Speicher, wenn er nur einige Felder benötigt? Diese Leistungsprobleme sind in der Entwicklung vielleicht nicht offensichtlich, werden aber bei Skalierung kritisch. Ich habe einmal Code überprüft, der in Ordnung zu sein schien, aber dazu geführt hätte, dass die CPU unserer Datenbank auf 100 % hochgefahren wäre, wenn er in die Produktion geschickt worden wäre, einfach weil er es nicht einmal geschafft hätte...