Code-Review-Kultur: Warum Sie eine brauchen

Wie oft drücken Sie beim Öffnen eines Pull Requests die Daumen in der Hoffnung, dass er nicht diesem einen Entwickler in Ihrem Team zugewiesen wird, der immer eine Handvoll Kommentare zu allem und jedem hinterlässt? Ich spreche nicht von den sehr vernünftigen Kommentaren, die gemachte Fehler oder, bitte nicht, Code-Smell aufdecken.

Dabei handelt es sich um Kommentare zu Ihrem Codestil oder um kleine subjektive Dinge, die nicht darauf abzielen, die Codebasis am Ende zu verbessern, sondern eher nach dem Motto „Ich hätte es anders gemacht, weil es mir einfach besser gefällt“ klingen. Als verantwortungsbewusster Teamkollege werden Sie diese wahrscheinlich ansprechen.

Egal, für welche Option Sie sich entscheiden – ob Sie einfach alle Änderungen übernehmen oder mit langen Erklärungen dagegenhalten, warum Sie mit diesem und jenem nicht einverstanden sind –, am Ende werden Sie sich wahrscheinlich aufgeregt, erschöpft und frustriert fühlen, weil Sie so viel Zeit mit unwichtigen Dingen verbracht haben. Und war es das wirklich wert? Diese Frage wird Ihnen jedes Mal in den Kopf kommen, wenn Sie mit solchen Situationen konfrontiert werden.

Oder manchmal ist es auch anders: Insta genehmigt ohne Kommentare und ohne Anzeichen dafür, dass ein Prüfer Ihre Änderungen tatsächlich sorgfältig geprüft hat. Ob sie es nun geprüft haben und es keine objektiven Eingaben gibt oder nicht, Sie müssen sich selbst und diesen Teamkollegen fragen.

Zugegeben, selbst wenn alles gut ist, wäre es nicht nett, dem Autor kurz zu danken und sicherzustellen, dass Sie auf derselben Seite stehen und keine Zweifel mehr bestehen?

Wenn Ihnen beide Geschichten oben bekannt vorkommen, ist dieser Artikel für Sie. Was Ihrem Team fehlt, ist eine Pull Request-Kultur oder anders gesagt eine Reihe von Richtlinien für die Kommunikation in Pull Requests, um einen freundlichen und hocheffizienten Zusammenarbeitsprozess zu unterstützen. Ich werde die wesentlichen Teile behandeln, nach denen Prüfer suchen sollten, und auch Beispiele für fragwürdige Kommentare – einfacher gesagt, Kleinigkeiten –, die Reibereien oder manchmal sogar Probleme jenseits Ihrer persönlichen Gefühle verursachen.

Da ich selbst iOS-Entwickler bin, verwende ich in meinen Codebeispielen Swift, aber grundsätzlich ist dieses Thema für jeden Entwickler wichtig, unabhängig von der Plattform oder Sprache, die Sie täglich verwenden.

Ich glaube jedoch, dass die Codeüberprüfungskultur für Apple-Plattformen noch relevanter ist, da wir meiner Erfahrung nach in vielerlei Hinsicht eher wählerisch sind. Wahrscheinlich kommt das von einer perfektionistischen Denkweise, die wir aus Steves Vision von früher übernommen haben.

Lektion gelernt

Pull Requests oder Merge Requests sind eine Möglichkeit, Codeänderungen mit Kollegen zu verifizieren und auf Fehler zu prüfen, die Bereitschaft zu beurteilen, bevor mit den nächsten Schritten fortgefahren wird, und sie schließlich an Endbenutzer weiterzuleiten. Es handelt sich auch um einen der wichtigsten Kommunikationskanäle zwischen Entwicklern. Manchmal kennen wir eine Person außerhalb der Threads in PRs nicht einmal.

Besonders in den frühen Phasen ihrer Karriere achten Entwickler häufig nicht auf ihre PR-Kommunikation. Insbesondere darauf, wie ihre Kommentare von anderen wahrgenommen werden, ob die vorgebrachten Punkte klar und korrekt auf Englisch formuliert sind usw.

Glauben Sie mir, ich habe das schon erlebt und auch Fehler gemacht. Eine Erfahrung ist mir bis heute im Gedächtnis geblieben. Ich bringe sie bei Diskussionen über PR-Kultur fast immer zur Sprache, weil sie die Wichtigkeit dieses Themas perfekt unterstreicht.

Als ich vor Jahren nach einem neuen Job suchte, wurde ich mit einem simulierten Pull Request konfrontiert, den ich überprüfen musste. Ich habe damals nicht gründlich über alles nachgedacht, weil ich mich darüber freute, eine einfache Aufgabe erhalten zu haben und nicht erneut mit Leetcode gefoltert zu werden. Natürlich war es nicht so einfach. Es war ein Test in Sachen Pull Request-Kultur, bei dem ich drastisch durchgefallen bin.

Ich habe den PR schnell überflogen und eine Menge größtenteils sinnloser Kommentare hinterlassen, zum Beispiel:

 import UIKit import AVFoundation // It would be nice to list your imports alphabetically, so that it's easier to read.

Oder eine andere:

 func someMethod() { // about 30 lines of code } // What do you think about splitting this method in half?

In anderen seltenen Kommentaren, in denen ich ein Problem entdeckte, war ich nicht deutlich genug und bot keine vernünftige Lösung an. Der Hauptfehler dieser Übung bestand jedoch darin, dass ich einige Leistungsprobleme übersah, weil ich mich nicht richtig konzentrierte. Am Ende erhielt ich folgendes Feedback:

Meiner Meinung nach haben Sie viele wichtige Details übersehen, etwa die Korrektheit des Codes, die Leistung und allgemeine Verbesserungen (mit einigen geringfügigen Ausnahmen).

Sie haben sich bei der Überprüfung stark auf die Formatierung konzentriert, was eine Teamdiskussion sein sollte und/oder Formatierungstools verwendet werden sollten. Dies ermöglicht es Entwicklern auch, sich bei einer Überprüfung auf die richtigen Dinge zu konzentrieren.

Seitdem wurde mir klar, dass mein Ansatz zur Codeüberprüfung alles andere als perfekt war und viele wichtige Dinge fehlten.

Dieses Beispiel ist in gewisser Weise recht primitiv, zeigt jedoch deutlich, wie das Fehlen einer PR-Kultur leicht zu einem gefährlichen Weg werden kann, der direkt zu Fehlern und Problemen in der Produktion führt.

Lassen Sie uns in die Details eintauchen und mit etwas beginnen, das Sie als Code-Prüfer vermeiden möchten: das Schreiben kleinlicher Kommentare.

Was ist ein Nitpick-Kommentar?

Es gibt viele Möglichkeiten, Ihre Logik und Gedanken in Code auszudrücken. Moderne Programmiersprachen bieten Ihnen dafür genügend Flexibilität und oft gibt es zwischen den verschiedenen Ansätzen kein Richtig oder Falsch. Wir möchten jedoch nicht, dass unsere Projekte wie Frankenstein-Monster aussehen – jedes Team sollte einen anständigen Codestil mit Richtlinien haben.

Gleichzeitig können und sollten wir nicht jede Codezeile unserer Teamkollegen kontrollieren. Ich glaube, dass es eine Tugend ist, die wir idealerweise anstreben sollten, ein gutes Gleichgewicht zwischen beiden zu finden.

Ein Nitpick-Kommentar ist eine Aufforderung, eine Änderung vorzunehmen, normalerweise eine kleine, für die es keine objektiven Gründe gibt. Oft handelt es sich dabei um die Projektion einer persönlichen Präferenz eines Code-Reviewers in den rezensierten Code. Lassen Sie mich einige Beispiele dafür zeigen:

 // Original guard let newValue, newValue != oldValue else { return } // Change request guard let newValue, newValue != oldValue else { return }

Wenn Sie mich fragen, würde ich immer die zweite Option wählen, da es meiner Meinung nach einfacher ist, solchen Code zu debuggen und einen Haltepunkt direkt in die Rückgabezeile zu setzen. Man kann aber argumentieren, dass die erste Variante 2 Zeilen spart, und außerdem sieht man diesen Stil auch bei den Apple-Ingenieuren.

 // Original func handleErrorIfNeeded(_ error: SomeError) { if error == .failedRequest { return } if error == .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases } // Change request func handleErrorIfNeeded(_ error: SomeError) { guard error != .failedRequest else { return } guard error != .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases }

In vielen logischen Situationen können guard und if in Swift synonym verwendet werden. Ich würde jedoch argumentieren, dass sie nicht die gleiche Sprachsemantik haben. guard möchte, wie der Name schon sagt, Ihren Codefluss vor einem unerwünschten Ergebnis „schützen“, während if von Natur aus neutral ist und eine 50-50%ige Chance für unterschiedliche Codepfade zulässt. Technisch gesehen sind beide Varianten jedoch nicht falsch und perfekt lesbar.

Und das Letzte:

 // Original, the constant value is not being repeated anywhere else func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(130) // … calculation return finalDuration } // Change request func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(Constant.startDuration) // … calculation return finalDuration } enum Constant { static let startDuration = Double(130) }

Man kann darüber streiten, ob es notwendig ist, jede Konstante in einen separaten Namespace zu extrahieren, wenn sie nur einmal verwendet wird und es sich nicht um eine magische Konstante handelt. In der ersten Variante wissen Sie genau, was diese Zahl 130 ist. Allerdings wird Ihnen jemand sagen, dass Sie jede Konstante wie diese extrahieren sollen, egal was passiert.

Bei allen obigen Beispielen handelt es sich um kleinliche Änderungen. Beide Varianten sind vollkommen in Ordnung, für jeden lesbar und beschädigen Ihren Code nicht. Es sind lediglich Alternativen mit gleichem Nutzungsgewicht.

Was also möchten Sie mit Kommentaren wie diesen tun? Aus Sicht eines Rezensenten werden einige so etwas sagen wie:

Nun, es ist nur ein Vorschlag. Es ist nicht zwingend, ihn anzuwenden.

Stimmt, aber gleichzeitig ist es wichtig, den Autor darüber zu informieren, zum Beispiel:

Es ist nur ein Vorschlag, Sie können ihn gern verwerfen, aber was halten Sie von diesem Ansatz: …?

Meine Meinung zu Nitpicks ist jedoch , sie überhaupt nicht zu veröffentlichen . Warum?

Nehmen wir an, Sie haben einen kleinlichen Kommentar abgegeben und sogar hervorgehoben, dass es sich um einen Vorschlag mit niedriger Priorität handelt. Was passiert dann?

• 1. Möglichkeit: Der Autor nimmt die Änderung vor. Was bedeutet das im Hintergrund wirklich?
  • Sie müssen die aktuellen Änderungen speichern, die sie vornehmen.
  • Als nächstes müssen sie in die PR-Branche wechseln.
  • Sie müssen die Änderung anwenden.
  • Sie könnten Konflikte lösen.
  • Sie müssen den Wandel vorantreiben.
  • Dann wird aufgrund des gepushten neuen Codes eine weitere Runde von CI-Tests ausgelöst.
  • Sie müssen auf den Kommentar mit Text oder Emoji antworten.

Einerseits handelt es sich um eine sehr einfache Reihe von Aufgaben, die wir als Entwickler täglich erledigen. Aber stellen Sie sich vor, es ist nicht ein Nitpick-Kommentar, sondern gleich 10. Oder Sie haben gleichzeitig 5 Pull Requests mit unterschiedlich vielen Nitpicks geöffnet. War das Ihre Zeit und den Aufwand wirklich wert? Wenn wir wissen, was ein Nitpick ist, lautet die Antwort nein. Ich würde meine Änderungen lieber schneller liefern und Zeit sparen.

• 2. Möglichkeit: Der Autor nimmt die Änderung nicht vor.

  • In vielen Situationen ist es einfach unhöflich, den Kommentar eines Teamkollegen zu ignorieren, ohne überhaupt zu antworten. Und es könnte auch unhöflich wirken, einfach mit Folgendem zu antworten:

    
    

    Danke für den Vorschlag, aber er gefällt mir nicht. Ich werde ihn nicht anwenden, da ich ihn nicht für notwendig halte.

  
  

  • Stattdessen werden Sie als Autor wahrscheinlich ausführlicher erklären, warum Sie die Änderungen nicht übernehmen werden. Beispielsweise Ihre Vision zu guard vs. let wie oben.

    
    

  • Dann wäre es großartig, wenn der Rezensent Ihre Meinung einfach akzeptiert und weitermacht. Aber er könnte auch den Widerstand spüren und reagieren, weil er wirklich glaubt, dass seine Methode besser ist.

    
    

  • Solche Threads können leicht ausufern, wenn man lange Kommentare hin und her schreibt, was überhaupt nicht produktiv ist. Und am Ende führt es nirgendwo hin. Sie können beide zustimmen, dass Sie anderer Meinung sind, es gibt keinen „Gewinner“, aber die endgültige Frage ist dieselbe. War es Ihre Zeit und den Aufwand wirklich wert?

  
  

Meine Praxis besteht darin, diese Kleinigkeiten bei Code-Überprüfungen herauszufiltern, noch bevor ich etwas poste, indem ich mir selbst diese Fragen stelle:

Ist es wirklich wichtig, etwas zu ändern?

Gibt es dafür objektive Gründe? Wenn ja, benennen Sie diese.

Möchte ich diesen Kommentar nur posten, weil ich es anders gemacht hätte? Oder liegt ein echtes Anliegen vor?

Auf diese Weise können Sie den „Lärm“ kleinlicher Kommentare aus Ihrer Codeüberprüfung entfernen und nur die wichtigen Teile belassen, also die Teile, bei denen Sie sich alle auf Skalierbarkeit, Robustheit, Thread-Sicherheit usw. konzentrieren müssen.

Aber warum erscheinen solche Kommentare? Normalerweise ist es ein Zeichen dafür, dass die Prüfer nicht genau wissen, worauf sie sich bei Pull Requests konzentrieren oder wonach sie suchen sollen. Im nächsten Abschnitt werden wir genau das besprechen.

So schaffen Sie eine Code-Review-Kultur

Sehen wir uns an, wie wir eine Blaupause für einen PR-„Goldstandard“ erstellen können. Ich werde eine Reihe von Werten und Richtlinien auflisten, die meiner Meinung nach für die Verbesserung Ihrer Codeüberprüfungen von wesentlicher Bedeutung sind:

1. Für alle: Definieren Sie eine Reihe von Buckets oder Kategorien für Ihre Codeüberprüfungskommentare und weisen Sie ihnen unterschiedliche Schweregrade zu. Zeigen Sie Ihren Kollegen immer Beispiele, damit Sie alle auf dem gleichen Stand sind. Beispiel:

…Und so weiter. In Ihrem Team kann das anders sein, aber die Idee hier ist, eine Struktur und Klassifizierung zu haben. Durch die Einbeziehung von Schweregradwerten können wir die Dinge besser priorisieren und uns auf das Wesentliche konzentrieren.

2. Prüfer : Versuchen Sie, für Ihre Codeüberprüfungen einen bestimmten Zeitrahmen zu reservieren. Überstürzen Sie nichts und gehen Sie tiefer auf die Bedeutung der Codeänderungen ein, nicht auf die Syntax. Dadurch sollten Sie die Qualität und Klarheit Ihrer Kommentare verbessern.

   
   

3. Autor: Helfen Sie Ihren Kollegen. Wenn Sie wissen, dass einige Teile aufgrund des fehlenden Kontexts unklar sein können oder weil Ihr PR nur ein Teil einer Reihe mit noch folgenden Dingen ist, hinterlassen Sie einfach im Voraus einen Kommentar mit einer Erklärung zu Ihrem eigenen Pull Request. Das spart beiden Seiten Zeit!

   
   

4. Gutachter: Lesen Sie Ihre Kommentare noch einmal durch, bevor Sie sie veröffentlichen. Versetzen Sie sich in die Lage der anderen Seite. Ist alles klar? Versuchen Sie, sich an die folgende Struktur zu halten:

   1. Schildern Sie Ihr Anliegen.
   2. Warum es wichtig ist.
   3. Was schlagen Sie stattdessen vor?

   
   

5. Für alle: Überprüfen Sie Ihre Grammatik und Rechtschreibung im Englischen. Ja, für viele von uns ist Englisch nicht unsere Muttersprache, und das ist manchmal schwierig. Aber wenn es nicht gut geschrieben ist, kann es zu allerlei Verwirrung und Fehlinterpretationen führen, die Sie nicht haben möchten.

   
   

6. Für alle: Wie kommt dein Kommentar an? Klingt er freundlich? Dieser Punkt ist dem vorherigen recht ähnlich, aber die Idee ist, in der schriftlichen Form einen gesunden, freundlichen Ansatz beizubehalten. Zum Beispiel:

   
   

   Sie müssen diese Logik in eine separate Methode extrahieren.

   
   

   Obwohl an diesem Satz grammatikalisch nichts auszusetzen ist, wird das Wort „muss“ in diesem Kontext selten verwendet und kann für den Leser als Befehl aufgefasst werden. Regierungsbeamte werden Ihnen sagen, was Sie tun müssen , weil es Gesetze gibt. Aber das ist nicht die Art von Formulierung, die Sie gegenüber Ihren Kollegen verwenden möchten, wenn Sie im selben Team sind. Versuchen Sie stattdessen Folgendes:

   
   

   Was halten Sie davon, diese Logik in eine separate Methode zu extrahieren?

   
   

7. Rezensent: Gleichen Sie Negatives mit Positivem aus. Wenn Sie eine Handvoll kritischer Kommentare hinterlassen haben, suchen Sie auch einige gute heraus und markieren Sie diese. Das ist eine Kleinigkeit, verbessert aber die Gesamtwahrnehmung in den Augen des Autors.

   
   

8. Gutachter: Wenn bei Codeänderungen alles gut ist, scheuen Sie sich nicht, Lob zu posten. Insta-Freigaben ohne Anzeichen sind kein gutes Verhalten und geben Anlass zu vielen Fragen. Posten Sie einen Shoutout mit den genauen Dingen, die Sie an der Arbeit Ihres Kollegen toll fanden. Einfaches „LGTM“ ist zu allgemein und kann als abweisend empfunden werden.

Ich denke, das sind gute Punkte, mit denen Sie in Ihrem Team beginnen können, um auf eine bessere und gesündere Kommunikation in Ihren Pull Requests hinzuarbeiten. Der schwierigere Teil danach besteht darin, diese Richtlinien weiterhin einzuhalten. Manchmal kann es zwei Schritte vorwärts und einen Schritt zurück sein.

Abschluss

Letzten Endes ist das Programmieren nicht immer der schwierigste Teil. Kommunikation und Zusammenarbeit können eine größere Herausforderung darstellen, insbesondere wenn alle aus unterschiedlichen Ländern und mit unterschiedlichen Hintergründen kommen und über eigene Erfahrungen und Gedanken verfügen.

Ich hoffe, dieser Einblick in die Code-Review-Kultur war hilfreich und vielleicht können Sie etwas Nützliches daraus mitnehmen.

Viel Glück und freundliche Code-Reviews an Sie alle!