Code Review Best Practices

Das Internet bietet eine Fülle von Material über Code-Reviews: über die Auswirkung von Code-Reviews auf die Unternehmenskultur, über formale Sicherheits-Reviews, kürzere Leitfäden, längere Checklisten, humanisierte Reviews, Gründe, warum Code-Reviews überhaupt durchgeführt werden, Best Practices, mehr Best Practices, Statistiken über die Effektivität von Code-Reviews beim Abfangen von Fehlern, und Beispiele von schief gelaufenen Code-Reviews. Oh, und natürlich gibt es auch Bücher. Langer Rede kurzer Sinn, dieser Blogpost stellt Palantirs Sicht auf Code Reviews vor. Organisationen mit einer tiefen kulturellen Abneigung gegenüber Peer-Reviews sollten vielleicht den hervorragenden Aufsatz von Karl E. Wiegers über Humanizing Peer Reviews lesen, bevor sie versuchen, diesem Leitfaden zu folgen.

Dieser Beitrag ist aus den Best Practices Guides unserer Java Code Quality Toolkette Baseline kopiert und deckt die folgenden Themen ab:

• Warum, was und wann man Code-Reviews durchführt
• Vorbereitung von Code für die Überprüfung
• Durchführung von Code-Reviews
• Beispiele für Code-Reviews

Wir führen Code-Reviews (CRs) durch, um die Code-Qualität zu verbessern und von positiven Auswirkungen auf die Team- und Unternehmenskultur zu profitieren. Zum Beispiel:

• Committer werden durch die Vorstellung einer Gruppe von Reviewern motiviert, die sich die Änderungsanfrage ansehen: der Committer neigt dazu, lose Enden zu bereinigen, TODOs zu konsolidieren und den Commit allgemein zu verbessern. Die Anerkennung von Code-Expertise durch Gleichgesinnte ist eine Quelle des Stolzes für viele Programmierer.
• Wissen zu teilen hilft Entwicklungsteams auf mehrere Arten:
  – Ein CR kommuniziert explizit hinzugefügte/geänderte/entfernte Funktionalität an Teammitglieder, die anschließend auf der geleisteten Arbeit aufbauen können.
  – Der Committer kann eine Technik oder einen Algorithmus verwenden, von dem die Reviewer lernen können. Ganz allgemein helfen Code-Reviews dabei, die Qualitätsmesslatte in der gesamten Organisation anzuheben.
  – Reviewer können Wissen über Programmiertechniken oder die Code-Basis besitzen, das helfen kann, die Änderung zu verbessern oder zu konsolidieren; zum Beispiel kann jemand anderes gleichzeitig an einer ähnlichen Funktion oder Korrektur arbeiten.
  – Positive Interaktion und Kommunikation stärkt die sozialen Bindungen zwischen den Teammitgliedern.
• Konsistenz in einer Code-Basis macht den Code einfacher zu lesen und zu verstehen, hilft Bugs zu verhindern und erleichtert die Zusammenarbeit zwischen regulären und wandernden Entwickler-Arten.
• Lesbarkeit von Code-Fragmenten ist für den Autor, dessen Geistes Kind es ist, schwer zu beurteilen, und für einen Prüfer, der nicht den vollen Kontext hat, leicht zu beurteilen. Lesbarer Code ist besser wiederverwendbar, fehlerfrei und zukunftssicher.
• Versehentliche Fehler (z.B. Tippfehler) sowie strukturelle Fehler (z.B. toter Code, Logik- oder Algorithmusfehler, Leistungs- oder Architekturprobleme) sind für kritische Reviewer mit einer Außenperspektive oft viel leichter zu erkennen. Studien haben ergeben, dass selbst kurze und informelle Code-Reviews einen signifikanten Einfluss auf die Codequalität und die Fehlerhäufigkeit haben.
• Compliance- und regulatorische Umgebungen verlangen oft Reviews. CRs sind eine großartige Möglichkeit, um häufige Sicherheitsfallen zu vermeiden. Wenn Ihr Feature oder Ihre Umgebung signifikante Sicherheitsanforderungen hat, wird es von einem Review durch die lokalen Sicherheitsbeauftragten profitieren (und wahrscheinlich auch erfordern).

Was soll überprüft werden

Es gibt keine ewig gültige Antwort auf diese Frage und jedes Entwicklungsteam sollte sich auf seinen eigenen Ansatz einigen. Einige Teams bevorzugen es, jede Änderung, die in den Hauptzweig eingebunden wird, zu überprüfen, während andere eine „Trivialitäts“-Schwelle haben werden, unter der eine Überprüfung nicht erforderlich ist. Der Kompromiss besteht zwischen der effektiven Nutzung der Zeit der Ingenieure (sowohl der Autoren als auch der Reviewer) und der Aufrechterhaltung der Codequalität. In bestimmten regulatorischen Umgebungen kann ein Code-Review sogar für triviale Änderungen erforderlich sein.

Code-Reviews sind klassenlos: Die ranghöchste Person im Team zu sein, bedeutet nicht, dass Ihr Code nicht überprüft werden muss. Selbst wenn, in dem seltenen Fall, der Code fehlerfrei ist, bietet das Review eine Gelegenheit für Mentorschaft und Zusammenarbeit, und, minimal, erweitert das Verständnis von Code in der Code-Basis.

Wann überprüfen

Code-Reviews sollten geschehen, nachdem automatisierte Prüfungen (Tests, Style, andere CI) erfolgreich abgeschlossen wurden, aber bevor der Code in den Mainline-Zweig des Repositorys zusammengeführt wird. Wir führen in der Regel keine formale Codeüberprüfung von aggregierten Änderungen seit der letzten Veröffentlichung durch.

Für komplexe Änderungen, die in den Mainline-Zweig als eine Einheit zusammengeführt werden sollen, aber zu groß sind, um in eine vernünftige CR zu passen, ziehen Sie ein gestapeltes CR-Modell in Betracht: Erstellen Sie einen primären Zweig feature/big-feature und eine Anzahl von sekundären Zweigen (z.B. feature/big-feature-apifeature/big-feature-testing, usw.), die jeweils eine Teilmenge der Funktionalität kapseln und die einzeln gegen den feature/big-feature Zweig code-reviewed werden. Sobald alle sekundären Zweige in feature/big-feature zusammengeführt sind, erstellen Sie eine CR für das Zusammenführen des letzteren in den Hauptzweig.

Vorbereitung des Codes für die Überprüfung

Es liegt in der Verantwortung des Autors, CRs einzureichen, die einfach zu überprüfen sind, um die Zeit und Motivation der Überprüfer nicht zu verschwenden:

• Umfang und Größe. Änderungen sollten einen engen, gut definierten, in sich geschlossenen Umfang haben, den sie erschöpfend abdecken. Eine Änderung kann zum Beispiel eine neue Funktion implementieren oder einen Fehler beheben. Kürzere Änderungen werden gegenüber längeren bevorzugt. Wenn eine CR substanzielle Änderungen an mehr als ~5 Dateien macht, oder länger als 1-2 Tage zum schreiben brauchte, oder mehr als 20 Minuten zur Überprüfung benötigen würde, sollten Sie in Betracht ziehen, sie in mehrere in sich geschlossene CRs aufzuteilen. Zum Beispiel kann ein Entwickler eine Änderung einreichen, die die API für eine neue Funktion in Form von Schnittstellen und Dokumentation definiert, und eine zweite Änderung, die Implementierungen für diese Schnittstellen hinzufügt.
• Reichen Sie nur vollständige, selbst überprüfte (per diff) und selbst getestete CRs ein. Um die Zeit der Reviewer zu sparen, testen Sie die eingereichten Änderungen (d.h., führen Sie die Test-Suite aus) und stellen Sie sicher, dass sie alle Builds sowie alle Tests und Code-Qualitätsprüfungen, sowohl lokal als auch auf den CI-Servern, bestehen, bevor Sie Reviewer zuweisen.
• Refactoring-Änderungen sollten das Verhalten nicht verändern; umgekehrt sollte eine verhaltensverändernde Änderung Refactoring und Code-Formatierungsänderungen vermeiden. Dafür gibt es mehrere gute Gründe:
  – Refactoring-Änderungen berühren oft viele Zeilen und Dateien und werden daher mit weniger Aufmerksamkeit geprüft. Unbeabsichtigte Verhaltensänderungen können in die Code-Basis durchsickern, ohne dass es jemand bemerkt.
  – Große Refactoring-Änderungen brechen Cherry-Picking, Rebasing und andere Magie der Versionsverwaltung. Es ist sehr mühsam, eine Verhaltensänderung rückgängig zu machen, die als Teil eines Repository-weiten Refactoring-Commits eingeführt wurde.
  – Teure menschliche Überprüfungszeit sollte auf die Programmlogik und nicht auf Stil-, Syntax- oder Formatierungsdebatten verwendet werden. Wir ziehen es vor, diese mit automatisierten Werkzeugen wie Checkstyle, TSLint, Baseline, Prettier, etc. zu lösen.

Commit-Nachrichten

Das folgende ist ein Beispiel für eine gute Commit-Nachricht, die einem weithin zitierten Standard folgt:

Capitalized, short (80 chars or less) summaryMore detailed explanatory text, if necessary. Wrap it to about 120 characters or so. In some contexts, the first
line is treated as the subject of an email and the rest of the text as the body. The blank line separating the
summary from the body is critical (unless you omit the body entirely); tools like rebase can get confused if you run
the two together.Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches
up with commit messages generated by commands like git merge and git revert.Further paragraphs come after blank lines.- Bullet points are okay, too

Versuchen Sie zu beschreiben, was der Commit ändert und wie er es tut:

> BAD. Don't do this.
Make compile again> Good.
Add jcsv dependency to fix IntelliJ compilation

Reviewer finden

Es ist üblich, dass der Committer ein oder zwei Reviewer vorschlägt, die mit der Code-Basis vertraut sind. Oft ist einer der Reviewer der Projektleiter oder ein Senior Ingenieur. Projekteigentümer sollten in Betracht ziehen, ihre Projekte zu abonnieren, um über neue CRs benachrichtigt zu werden. Code-Reviews zwischen mehr als drei Parteien sind oft unproduktiv oder sogar kontraproduktiv, da verschiedene Reviewer möglicherweise widersprüchliche Änderungen vorschlagen. Dies kann auf eine grundsätzliche Uneinigkeit über die richtige Umsetzung hindeuten und sollte außerhalb eines Code-Reviews in einem Forum mit höherer Bandbreite geklärt werden, zum Beispiel persönlich oder in einer Videokonferenz mit allen Beteiligten.

Durchführen von Code-Reviews

Ein Code-Review ist ein Abstimmungspunkt zwischen verschiedenen Teammitgliedern und hat daher das Potenzial, den Fortschritt zu blockieren. Folglich müssen Code-Reviews zeitnah durchgeführt werden (in der Größenordnung von Stunden, nicht von Tagen), und Teammitglieder und Leiter müssen sich der zeitlichen Belastung bewusst sein und die Review-Zeit entsprechend priorisieren. Wenn Sie denken, dass Sie ein Review nicht rechtzeitig fertigstellen können, lassen Sie es den Committer sofort wissen, damit sie jemand anderen finden können.

Ein Review sollte gründlich genug sein, dass der Reviewer die Änderung mit einem vernünftigen Maß an Detail einem anderen Entwickler erklären kann. Das stellt sicher, dass die Details der Code-Basis mehr als nur einer Person bekannt sind.

Als Reviewer ist es Ihre Verantwortung, die Coding-Standards durchzusetzen und die Qualitätsleiste hoch zu halten. Code-Reviewing ist eher eine Kunst als eine Wissenschaft. Der einzige Weg es zu lernen, ist es zu tun; ein erfahrener Reviewer sollte in Betracht ziehen, andere weniger erfahrene Reviewer auf ihre Änderungen anzusetzen und sie zuerst ein Review machen zu lassen. Unter der Annahme, dass der Autor die oben genannten Richtlinien befolgt hat (besonders in Bezug auf die Selbstüberprüfung und die Sicherstellung, dass der Code läuft), ist hier eine Liste von Dingen, auf die ein Reviewer bei einem Code-Review achten sollte:

Zweck

• Erfüllt dieser Code den Zweck des Autors? Jede Änderung sollte einen bestimmten Grund haben (neues Feature, Refactor, Bugfix, etc.). Erfüllt der eingereichte Code tatsächlich diesen Zweck?
• Stellen Sie Fragen. Funktionen und Klassen sollten aus einem bestimmten Grund existieren. Wenn der Grund für den Prüfer nicht klar ist, kann das ein Hinweis darauf sein, dass der Code umgeschrieben oder mit Kommentaren oder Tests unterstützt werden muss.

Implementierung

• Überlegen Sie, wie Sie das Problem gelöst hätten. Wenn es anders ist, warum ist das so? Behandelt Ihr Code mehr (Rand-)Fälle? Ist er kürzer/einfacher/sauberer/schneller/sicherer und dennoch funktional gleichwertig? Gibt es ein zugrundeliegendes Muster, das Sie entdeckt haben, das vom aktuellen Code nicht erfasst wird?
• Sehen Sie Potenzial für nützliche Abstraktionen? Teilweise duplizierter Code deutet oft darauf hin, dass ein abstrakteres oder allgemeineres Stück Funktionalität extrahiert und dann in verschiedenen Kontexten wiederverwendet werden kann.
• Denken Sie wie ein Gegner, aber seien Sie nett dabei. Versuchen Sie, Autoren zu „erwischen“, die Abkürzungen nehmen oder Fälle auslassen, indem Sie problematische Konfigurationen/Eingabedaten vorschlagen, die ihren Code kaputt machen.
• Denken Sie an Bibliotheken oder bestehenden Produktcode. Wenn jemand eine bestehende Funktionalität neu implementiert, geschieht das oft nur, weil er nicht weiß, dass sie bereits existiert. Manchmal wird Code oder Funktionalität absichtlich dupliziert, z.B. um Abhängigkeiten zu vermeiden. In solchen Fällen kann ein Code-Kommentar die Absicht verdeutlichen. Wird die eingeführte Funktionalität bereits von einer bestehenden Bibliothek bereitgestellt?
• Folgt die Änderung Standardmustern? Etablierte Codebasen weisen oft Muster in Bezug auf Namenskonventionen, Dekomposition der Programmlogik, Datentypdefinitionen usw. auf. Es ist in der Regel wünschenswert, dass Änderungen in Übereinstimmung mit bestehenden Mustern implementiert werden.
• Fügt die Änderung Kompilierzeit- oder Laufzeitabhängigkeiten hinzu (insbesondere zwischen Teilprojekten)? Wir wollen unsere Produkte lose gekoppelt halten, mit so wenig Abhängigkeiten wie möglich. Änderungen an Abhängigkeiten und dem Build-System sollten sehr genau geprüft werden.

Lesbarkeit und Stil

• Denken Sie an Ihre Leseerfahrung. Haben Sie die Konzepte in einer angemessenen Zeit erfasst? War der Lesefluss in Ordnung und waren die Namen der Variablen und Methoden leicht zu verstehen? Konnten Sie bei mehreren Dateien oder Funktionen den Überblick behalten? Wurden Sie durch inkonsistente Namensgebung abgeschreckt?
• Hält sich der Code an die Kodierrichtlinien und den Codestil? Ist der Code konsistent mit dem Projekt in Bezug auf Stil, API-Konventionen usw.? Wie bereits erwähnt, ziehen wir es vor, Stildebatten mit automatisierten Werkzeugen zu klären.
• Hat der Code TODOs? TODOs stapeln sich im Code und werden mit der Zeit schal. Lassen Sie den Autor ein Ticket auf GitHub Issues oder JIRA einreichen und die Issue-Nummer an das TODO anhängen. Die vorgeschlagene Codeänderung sollte keinen auskommentierten Code enthalten.

Wartbarkeit

• Lesen Sie die Tests. Wenn es keine Tests gibt und es sollte welche geben, bitten Sie den Autor, welche zu schreiben. Wirklich untestbare Funktionen sind selten, während ungetestete Implementierungen von Funktionen leider häufig sind. Prüfen Sie die Tests selbst: decken sie interessante Fälle ab? Sind sie lesbar? Verringert die CR die gesamte Testabdeckung? Überlegen Sie, wie der Code brechen könnte. Stilstandards für Tests sind oft anders als der Kerncode, aber trotzdem wichtig.
• Bringt diese CR das Risiko mit sich, Testcode, Staging Stacks oder Integrationstests zu brechen? Diese werden oft nicht als Teil der Pre-Commit/Merge-Checks überprüft, aber wenn sie kaputt gehen, ist das für alle schmerzhaft. Spezifische Dinge, auf die man achten sollte, sind: Entfernung von Test-Utilities oder Modi, Änderungen in der Konfiguration und Änderungen im Layout/der Struktur von Artefakten.
• Bricht diese Änderung die Abwärtskompatibilität? Wenn ja, ist es in Ordnung, die Änderung zu diesem Zeitpunkt einzubinden oder sollte sie in eine spätere Version verschoben werden? Zu den Brüchen können Datenbank- oder Schemaänderungen, öffentliche API-Änderungen, Änderungen am Benutzer-Workflow usw. gehören.
• Braucht dieser Code Integrationstests? Manchmal kann Code nicht ausreichend mit Unit-Tests allein getestet werden, besonders wenn der Code mit externen Systemen oder Konfigurationen interagiert.
• Hinterlassen Sie Feedback zu Dokumentation, Kommentaren und Commit-Meldungen auf Code-Ebene. Redundante Kommentare verunreinigen den Code, und knappe Commit-Nachrichten verwirren zukünftige Mitwirkende. Das ist nicht immer zutreffend, aber qualitativ hochwertige Kommentare und Commit-Nachrichten werden sich im Laufe der Zeit bezahlt machen. (Denken Sie an eine Zeit, in der Sie eine exzellente, oder wirklich schreckliche, Commit Nachricht oder Kommentar gesehen haben.)
• Wurde die externe Dokumentation aktualisiert? Wenn Ihr Projekt ein README, CHANGELOG oder eine andere Dokumentation unterhält, wurde diese aktualisiert, um die Änderungen widerzuspiegeln? Veraltete Dokumentation kann verwirrender sein als keine, und es wird teurer sein, sie in der Zukunft zu reparieren, als sie jetzt zu aktualisieren.

Vergessen Sie nicht, prägnanten/lesbaren/effizienten/eleganten Code zu loben. Umgekehrt ist es nicht unhöflich, eine CR abzulehnen oder zu missbilligen. Wenn die Änderung redundant oder irrelevant ist, lehnen Sie sie mit einer Erklärung ab. Wenn Sie sie aufgrund eines oder mehrerer fataler Fehler für inakzeptabel halten, lehnen Sie sie ab, wiederum mit einer Erklärung. Manchmal ist das richtige Ergebnis einer CR „lasst uns das ganz anders machen“ oder sogar „lasst uns das überhaupt nicht machen.“

Sein Sie respektvoll gegenüber den Reviewern. Obwohl kontradiktorisches Denken praktisch ist, ist es nicht Ihre Funktion und Sie können nicht alle Entscheidungen treffen. Wenn Sie mit dem Code, so wie er ist, keine Einigung mit Ihrem Prüfer erzielen können, wechseln Sie zur Echtzeit-Kommunikation oder holen Sie eine dritte Meinung ein.

Sicherheit

Überprüfen Sie, ob API-Endpunkte eine angemessene Autorisierung und Authentifizierung durchführen, die mit dem Rest der Code-Basis übereinstimmt. Prüfen Sie auf andere häufige Schwachstellen, z. B. schwache Konfiguration, böswillige Benutzereingaben, fehlende Log-Events usw. Im Zweifelsfall verweisen Sie den CR an einen Experten für Anwendungssicherheit.

Kommentare: prägnant, freundlich, umsetzbar

Reviews sollten prägnant und in neutraler Sprache geschrieben sein. Kritisieren Sie den Code, nicht den Autor. Wenn etwas unklar ist, fragen Sie nach einer Klärung, anstatt Unwissenheit zu unterstellen. Vermeiden Sie Possessivpronomen, insbesondere in Verbindung mit Bewertungen: „mein Code hat vor Ihrer Änderung funktioniert“, „Ihre Methode hat einen Fehler“, usw. Vermeiden Sie absolute Urteile: „das kann nie funktionieren“, „das Ergebnis ist immer falsch“.

Versuchen Sie zu unterscheiden zwischen Vorschlägen (z.B. „Vorschlag: Methode extrahieren, um die Lesbarkeit zu verbessern“), erforderlichen Änderungen (z.B. „@Override hinzufügen“) und Punkten, die einer Diskussion oder Klärung bedürfen (z.B. „Ist das wirklich das richtige Verhalten? Wenn ja, fügen Sie bitte einen Kommentar hinzu, der die Logik erklärt.“). Ziehen Sie in Erwägung, Links oder Verweise auf ausführliche Erklärungen eines Problems bereitzustellen.

Wenn Sie mit einer Code-Überprüfung fertig sind, geben Sie an, inwieweit Sie erwarten, dass der Autor auf Ihre Kommentare reagiert und ob Sie den CR nach der Implementierung der Änderungen erneut überprüfen möchten (z.B., „

Reagieren auf Reviews

Teil des Zwecks des Code Reviews ist es, den Änderungswunsch des Autors zu verbessern; seien Sie daher nicht beleidigt über die Vorschläge Ihres Reviewers und nehmen Sie sie ernst, auch wenn Sie nicht zustimmen. Reagieren Sie auf jeden Kommentar, auch wenn es nur ein einfaches „ACK“ oder „done“ ist. Erläutern Sie, warum Sie bestimmte Entscheidungen getroffen haben, warum es eine Funktion gibt, usw. Wenn Sie sich nicht mit dem Prüfer einigen können, wechseln Sie zur Echtzeit-Kommunikation oder holen Sie eine externe Meinung ein.

Fixes sollten in den gleichen Zweig gepusht werden, aber in einem separaten Commit. Commits während des Review-Prozesses zu quetschen, macht es für den Reviewer schwer, Änderungen nachzuverfolgen.

Unterschiedliche Teams haben unterschiedliche Merge-Richtlinien: Einige Teams erlauben nur den Projekteigentümern das Mergen, während andere Teams es dem Mitwirkenden erlauben, nach einem positiven Code-Review zu mergen.

Persönliche Code-Reviews

Für die Mehrheit der Code-Reviews sind asynchrone Diff-basierte Tools wie Reviewable, Gerrit oder GitHub eine gute Wahl. Komplexe Änderungen oder Reviews zwischen Parteien mit sehr unterschiedlichem Fachwissen oder Erfahrung können effizienter sein, wenn sie persönlich durchgeführt werden, entweder vor demselben Bildschirm oder Projektor oder aus der Ferne über VTC oder Screen-Sharing-Tools.

Beispiele

In den folgenden Beispielen werden vorgeschlagene Review-Kommentare durch //R: ...-Kommentare in den Codeblöcken angezeigt.

Inkonsistente Namensgebung

class MyClass {
private int countTotalPageVisits; //R: name variables consistently
private int uniqueUsersCount;
}

Inkonsistente Methodensignaturen

interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<String> extractString(String s); /** Returns null if {@code s} cannot be rewritten. */
//R: should harmonize return values: use Optional<> here, too
public String rewriteString(String s);
}

Bibliotheksnutzung

//R: remove and replace by Guava's MapJoiner
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

Persönlicher Geschmack

int dayCount; //R: nit: I usually prefer numFoo over fooCount; up to you, but we should keep it consistent in this project

Fehler

//R: This performs numIterations+1 iterations, is that intentional?
// If it is, consider changing the numIterations semantics?
for (int i = 0; i <= numIterations; ++i) {
...
}

Architektonische Bedenken

otherService.call(); //R: I think we should avoid the dependency on OtherService. Can we discuss this in person?