Code Review Checklist: What I Look for After 10 Years of PRs \u2014 COD-AI.com

Vor drei Jahren genehmigte ich eine Pull-Request, die meinem Unternehmen am Ende eines einzigen Wochenendes 47.000 $ an verlorenen Einnahmen kostete. Der Code sah gut aus. Die Tests bestanden. Die Logik schien solide. Aber ich habe etwas Subtiles übersehen, wie wir Zeitzonenumrechnungen für unser Abrechnungssystem verwalteten, und Kunden in bestimmten Regionen wurden doppelt belastet.

Dieser Vorfall veränderte für immer, wie ich Code überprüfe. Ich bin Sarah Chen, und ich war in den letzten zehn Jahren Senior Engineering Manager bei drei verschiedenen SaaS-Unternehmen, in denen ich durchschnittlich 15-20 Pull-Requests pro Woche überprüfte. Das sind ungefähr 8.000 PRs in meiner Karriere. Ich habe brillanten Code gesehen, der Bugs erzeugte, und chaotischen Code, der jahrelang fehlerfrei in der Produktion lief. Ich habe gelernt, dass Code-Überprüfung nicht darum geht, Syntaxfehler zu finden – das erledigt Ihr Linter. Es geht darum, die unsichtbaren Probleme zu erfassen, die nur Erfahrung offenbaren kann.

Dieser Artikel ist die Checkliste, die ich mir gewünscht hätte, als ich anfing. Sie ist nicht umfassend – keine Checkliste könnte das – aber sie stellt die Muster dar, die ich gelernt habe zu erkennen, die Fragen, die ich mir angewöhnt habe zu stellen, und die mentalen Modelle, die meine Teams vor zahllosen Produktionsvorfällen bewahrt haben.

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

Bevor ich auch nur eine Zeile Code anschaue, verbringe ich 30 Sekunden damit, die PR-Beschreibung zu lesen. Das mag rückwärts erscheinen, aber ich habe festgestellt, dass sich 60 % der problematischen PRs sofort durch schlechte Kommunikation offenbaren. Eine gute PR-Beschreibung beantwortet drei Fragen: Was hat sich geändert, warum hat es sich geändert, und was könnte schiefgehen.

Wenn ich eine Beschreibung sehe, die sagt "Fehler behoben" oder "Komponente aktualisiert", weiß ich sofort, dass diese Überprüfung länger dauern wird. Der Autor hat nicht über die Auswirkungen seiner Änderungen nachgedacht, was bedeutet, dass ich diese Arbeit für ihn erledigen muss. Umgekehrt weiß ich, wenn ich eine Beschreibung sehe, die sagt: "Benutzerauthentifizierung umgestaltet, um JWT-Token anstelle von Sitzungscookies zu verwenden. Dies reduziert die Datenbanklast während der Spitzenzeiten um 40 %, erfordert jedoch, dass die Clients das Token-Refresh verwalten. Risiko: Bestehende mobile App-Versionen (< 2.3) müssen sich erneut authentifizieren," dass der Autor seine Hausaufgaben gemacht hat.

Ich achte auf spezifische Metriken in der Beschreibung. Nicht vage Verbesserungen, sondern tatsächliche Zahlen. "Leistung verbessert" bedeutet nichts. "Die API-Antwortzeit für den Benutzerprofil-Endpunkt unter 1000 gleichzeitigen Anfragen von 340 ms auf 180 ms reduziert" sagt mir, dass der Autor seine Arbeit gemessen hat und die Auswirkungen versteht. Meiner Erfahrung nach schreiben Entwickler, die Metriken in ihren PR-Beschreibungen einfügen, insgesamt nachdenklicheren Code.

Die Beschreibung sollte auch auf relevante Kontexte verweisen – das Ticket, das Entwurfsdokument, den Slack-Thread, in dem der Ansatz diskutiert wurde. Wenn ich eine PR isoliert überprüfe, ohne das breitere Umfeld zu verstehen, werde ich Dinge übersehen. Einmal genehmigte ich eine "einfache Umgestaltung", die unwissentlich eine kritische Integration brach, weil ich nichts über eine Abhängigkeit wusste, die nirgendwo dokumentiert war, außer in einem drei Monate alten E-Mail-Thread.

Roten Flaggen in PR-Beschreibungen sind: keine Beschreibung, Beschreibungen, die länger sind als die Codeänderungen selbst (deutet gewöhnlich auf Überengineering hin), Beschreibungen, die "WIP" oder "Entwurf" sagen, aber die PR ist als bereit zur Überprüfung gekennzeichnet, und Beschreibungen, die Phrasen wie "nicht sicher, ob dies der richtige Ansatz ist" enthalten (warum fordern Sie mich dann auf, sie zu überprüfen?).

Größe zählt: Die 400-Zeilen-Regel

Ich habe eine strenge Regel: Ich überprüfe PRs mit mehr als 400 Zeilen tatsächlicher Codeänderungen (ausgenommen generierter Code, package-lock-Dateien und Testfixtures) nicht gründlich. Das ist nicht willkürlich. Forschungen von Cisco und SmartBear zeigen, dass die Effektivität der Code-Überprüfung nach 400 Zeilen dramatisch abnimmt, und meine eigene Erfahrung bestätigt dies. Nach der Überprüfung von etwa 200 Zeilen Code beginnt mein Gehirn, Details zu ignorieren. Bei 400 Zeilen skimmed ich sie im Grunde.

"Code-Überprüfung geht nicht darum, Syntaxfehler zu finden – das erledigt Ihr Linter. Es geht darum, die unsichtbaren Probleme zu erfassen, die nur Erfahrung offenbaren kann."

Wenn jemand eine 1.200-Zeilen-PR einreicht, bitte ich ihn, sie aufzuteilen. Es ist mir egal, ob sie "alle zugehörig" sind. Große PRs sind der Ort, an dem sich Bugs verstecken. Ich habe kritische Sicherheitsanfälligkeiten in massiven Refactoring-PRs aus dem Auge verloren, weil die Prüfer müde wurden und aufhörten, bei Zeile 600 aufmerksam zu sein. Die Sicherheitsanfälligkeit war in Zeile 847.

Es gibt natürlich Ausnahmen. Manchmal müssen Dateien verschoben werden, oder generierter Code aktualisiert werden, oder umfassende Änderungen vorgenommen werden, um einen neuen Linting-Standard zu erfüllen. In solchen Fällen bitte ich den Autor, die mechanischen Änderungen von den logischen Änderungen zu trennen. Reichen Sie die Dateiänderungen in einer PR ein, die eigentlichen Logikänderungen in einer anderen. Das macht beide PRs einfacher zu überprüfen und einfacher zurückzusetzen, falls etwas schiefgeht.

Ich achte auch auf das Verhältnis von hinzugefügtem Code zu gelöscht. Meiner Erfahrung nach haben die besten PRs eine negative Nettozeilenanzahl – sie erreichen etwas, während sie den Codebestand verkleinern. Wenn ich eine PR sehe, die 500 Zeilen hinzufügt und 50 löscht, werde ich misstrauisch. Fügen wir Komplexität hinzu? Duplizieren wir bestehende Funktionen? Ist sich der Autor bewusst, was bereits im Codebestand vorhanden ist?

Das Größenproblem erstreckt sich auch auf einzelne Dateien. Wenn eine PR 30 verschiedene Dateien berührt, selbst wenn die gesamte Zeilenanzahl angemessen ist, ist das ein rotes Signal. Es deutet darauf hin, dass die Änderung entweder schlecht umrissen ist oder der Codebestand Kopplungsprobleme hat. So oder so verdient es besondere Aufmerksamkeit.

Die Business-Logik-Schicht: Wo die meisten Bugs leben

Nach einem Jahrzehnt der Überprüfung von Code kann ich Ihnen sagen, wo Bugs sich verstecken: in der Business-Logik-Schicht, speziell in den Randfällen und Zustandsübergängen. Nicht in den UI-Komponenten. Nicht in den Datenbankabfragen. In dem Code, der Ihre tatsächlichen Geschäftsregeln implementiert.

Wenn ich die Business-Logik überprüfe, achte ich auf Annahmen. Jede Annahme ist ein potenzieller Bug. Einmal überprüfte ich Code für ein Rabattberechnungssystem, das annahm, dass alle Rabatte Prozentsätze waren. Der Code funktionierte perfekt, bis das Marketing eine "10 $ Rabatt"-Aktion anbieten wollte. Das System stürzte ab, weil jemand durch den Rabattbetrag teilte, und die Division durch 10, wenn Sie eine Zahl zwischen 0 und 1 erwarten, ergibt sehr falsche Ergebnisse.

Ich frage mich: Was passiert an den Grenzen? Was passiert, wenn der Eingang null ist? Was passiert, wenn er negativ ist? Was passiert, wenn es null ist? Was ist der Unterschied zwischen einem leeren String und einem null String? Was passiert, wenn das Array leer ist? Was passiert, wenn der Benutzer keine Berechtigungen hat? Was passiert, wenn er alle Berechtigungen hat? Was passiert, wenn die Datenbank keine Ergebnisse zurückgibt? Was passiert, wenn sie eine Million Ergebnisse zurückgibt?

Ich achte auf magische Zahlen in der Business-Logik. Wenn ich if (user.loginAttempts > 5) sehe, frage ich: Warum 5? Ist das dokumentiert? Ist es konfigurierbar? Was passiert, wenn wir es für bestimmte Benutzertypen auf 3 ändern wollen? Magische Zahlen in der Business-Logik sind technische Schulden, die darauf warten, dass sie entstehen.

Zustandsmaschinen sind eine weitere häufige Quelle von Bugs. Wenn ich Code sehe, der Zustandsübergänge verwaltet – Bestellstatus, Benutzerlebenszyklus, Workflow-Stufen – zeichne ich das Zustandsdiagramm in meinem Kopf auf. Sind alle Übergänge berücksichtigt? Kann man in einen ungültigen Zustand gelangen? Ich habe einmal einen Bug entdeckt, bei dem ein Benutzer gleichzeitig "aktiv" und "ausgesetzt" sein konnte, weil der Code, der einen Status setzte, den anderen nicht überprüfte.

Ich achte auch auf Business-Logik, die sich über mehrere Ebenen erstreckt. Wenn ich Validierung in der UI sehe, weitere Validierung in der API und noch mehr Validierung in der Datenbankschicht, weiß ich, dass wir Inkonsistenzen haben werden. Geschäftsregeln sollten an einem Ort leben, und dieser Ort sollte offensichtlich sein.