Das Hauptziel von Code Reviews ist es, schwer wartbaren Code zu finden

 (mathstodon.xyz)
5 Punkte von GN⁺ 5 시간 전 | 1 Kommentare | Auf WhatsApp teilen
  • Code Reviews sind weniger ein Verfahren, um Bugs aufzuspüren oder Integrität zu garantieren, sondern eher ein Prozess, der später schwer wartbaren Code frühzeitig sichtbar macht
  • Wenn Reviewer den Code lesen und ihn nur schwer verstehen können, ist die Wahrscheinlichkeit hoch, dass künftige Maintainer auf dasselbe Problem stoßen
  • Änderungen sollten besser jetzt vorgenommen werden, solange die ursprünglichen Autorinnen und Autoren den Kontext noch im Kopf haben; die Erwartung, Bugs allein durch Code-Inspektion zuverlässig zu finden, ist unrealistisch
  • Statt „findet Bugs“ ist „versucht, es zu verstehen, und markiert die Stellen, die ihr nicht versteht“ eine für Reviewer besser umsetzbare Aufgabe
  • Ein gutes Review besteht nicht darin, alles perfekt zu beweisen, sondern beginnt damit, an unverständlichen Stellen Notizen zu hinterlassen und Verbesserungen einzufordern

Den Fokus von Code Reviews ändern

  • Der zentrale Zweck eines Code Reviews ist nicht, dass Reviewer Bugs finden, und auch nicht, zu garantieren, dass der Code bugfrei ist
  • Die Erwartung, durch bloßes Durchsehen von Code generell Bugs finden zu können, ist in der Praxis schwach
  • Deshalb liegt der Schwerpunkt des Reviews weniger auf „ist der Code korrekt?“ als auf „können andere ihn später lesen und ändern?“

Schwer verständlicher Code ist ein Warnsignal für die Wartbarkeit

  • Reviewer lesen Code mit dem Ziel zu verstehen, was er tut und wie er funktioniert
  • Unverständliche Stellen werden zu einem Warnsignal dafür, dass künftige Maintainer dort hängen bleiben könnten
  • Solchen Code sollte man besser sofort korrigieren, solange die ursprünglichen Autorinnen und Autoren den Kontext noch im Gedächtnis haben

Eine praktikablere Review-Aufgabe als Bugsuche

  • Die Aufforderung „finde Bugs in diesem Code“ ist eine Aufgabe, deren Erfolg schwer zu beurteilen ist
  • Selbst wenn man einige Bugs findet, könnten weitere versteckte Bugs übersehen worden sein; aus Sicht der Reviewer wird oft nur ein Scheitern klar sichtbar
  • Umgekehrt hat die Aufforderung „versuche, diesen Code zu verstehen, und weise darauf hin, wenn du etwas nicht verstehen kannst“ klarere Kriterien
    • Es ist nicht nötig, alles perfekt zu verstehen
    • Es reicht, die nicht verstandenen Stellen festzuhalten
    • Wer versucht, das Ganze zu verstehen, und an den Stellen, an denen er hängen bleibt, Notizen hinterlässt, erfüllt die Rolle des Reviews

Review-Kriterien in der Praxis

  • Code, den Reviewer nicht verstehen, kann schon für sich genommen ein Änderungskandidat sein
  • Review-Kommentare dienen nicht nur als Bugreports, sondern machen auch fehlende Erklärungen, strukturelle Probleme und schwer lesbare Abläufe sichtbar
  • Nach diesem Maßstab ist es wichtiger, den Code in einen Zustand zu bringen, in dem spätere Teammitglieder ihn lesen und bearbeiten können, als seine Korrektheit zu beweisen

Bugfunde sind ein Nebeneffekt

  • Das bedeutet nicht, dass Code Reviews gar keine Bugs finden
  • Bugs können während eines Reviews entdeckt werden, aber als Methode, um alle oder die meisten Bugs zu finden, sollte man sie kaum erwarten
  • Eine realistischere Erfolgsbedingung ist, die Verständlichkeit zu prüfen und schwer wartbare Stellen gemeinsam mit den ursprünglichen Autorinnen und Autoren sofort zu verbessern

Verwandte Beiträge

1 Kommentare

 
GN⁺ 5 시간 전
Meinungen auf Hacker News

  • Code Reviews haben nicht nur einen einzigen Zweck. Schwer wartbaren Code zu finden, ist wichtig, aber es ist weder der einzige Zweck noch vielleicht der wichtigste.
    Sie dienen als Sicherheitsmechanismus, der es erschwert, schädlichen Code zu mergen, selbst wenn Entwickler oder KI in eine seltsame Richtung laufen, und jemand, der dem Problem nicht zu nahe ist, kann bessere Ansätze oder übersehene Probleme erkennen.
    Jemand, der andere Teile des Systems besser kennt, kann auch Interaktionsprobleme aufdecken, mindestens eine weitere Person wird mit dem Code vertraut, und sowohl Autor als auch Reviewer bekommen eine Lerngelegenheit.
    Das ist besonders wichtig, wenn es Erfahrungsunterschiede gibt: Beim Mentoring neuer Mitarbeiter füge ich sie als Reviewer zu all meinen PRs hinzu, damit sie sehen, wie ich arbeite, und reviewe auch ihre PRs, um ihnen Orientierung zu geben. Manchmal lerne auch ich von ihnen.
    Bugs zu finden gehört ebenfalls dazu, sollte aber nicht der Hauptmechanismus sein; besonders wichtig ist es bei Security- und Performance-Bugs, die sich mit automatisierten Tests nur schwer finden lassen.

    • Als ein weiteres, älteres Argument: Menschen schreiben Code anders, wenn sie wissen, dass ihr Code reviewed wird und dass sich anhand dieses Codes ein langfristiger Eindruck von der Kompetenz und organisatorischen Passung des Einreichenden bildet.
    • Ähnlich wie bei Punkt 3 kann jemand, der mit der jeweiligen Subdomäne vertrauter ist, Stellen erkennen, die nicht zum bestehenden Code passen oder problematisch werden könnten.
    • Ich habe „ein einziger Zweck“ so gelesen, dass man den Code verstehen und alles, was man nicht versteht, hinterfragen soll. Wenn damit gemeint ist, dass man nach dem Verstehen auf falsche, dumme oder unsichere Stellen hinweisen kann, ergibt das für mich Sinn.
      Das gilt besonders bei Modularisierung und Zerlegung: Wenn man einen riesigen PR als Ganzes verstanden hat, entsteht im Kopf ein Modell, und man beginnt zu sehen, ob er wartbar sein wird, irgendwann zum absoluten Albtraum wird oder irgendwo dazwischen liegt.

  • Der vielleicht wichtigste Teil von Code Reviews ist meiner Meinung nach Wissenstransfer.
    In unserem kleinen Team geben, außer in dringenden Fällen, alle Teammitglieder vor dem Merge ihre Zustimmung zu einem PR. Dadurch weiß das ganze Team grob, in welchem Zustand die Codebase gerade ist. So vermeiden wir Überraschungen wie „das gesamte System, von dem ich abhängig war, ist verschwunden“, wie ich sie früher in stärker siloisierten Unternehmen erlebt habe.
    Außerdem ist es ein Ort, um Fragen zur Funktionsweise zu stellen und Verständnis aufzubauen. In einem gut funktionierenden Team sollte jeder Entwickler das Gesamtsystem bis zu einem gewissen Grad verstehen, einschließlich der Teile, die er nicht direkt anfasst.
    Eine weitere wichtige Funktion ist die Überprüfung von Organisationswissen. Kürzlich habe ich eine Tabelle etwas geändert, und ein Kollege wies mich darauf hin, dass ein Microservice, den ich nicht berücksichtigt hatte, in diese Tabelle schreibt und dadurch kaputtgehen würde. Ich wusste weder, dass dieser Microservice existiert, noch dass er auf diese Tabelle zugreift, aber diese Überprüfung hat ein größeres Problem und eine Datenbereinigungssituation verhindert.

    • „Das ganze kleine Team gibt vor dem Merge eines PR seine Zustimmung“ klingt für eine kleine, langsam voranschreitende Codebase gut, wird aber, wenn man es einem größeren Team oder einer schnelleren Codebase aufzwingt, leicht zu einem formalen Ritual, bei dem Leute den Code flüchtig überfliegen und auf Approve klicken.
      Am Ende sind alle mit ihrer eigenen Arbeit beschäftigt und wollen nicht die Person sein, die einen wichtigen PR blockiert, also klicken sie einfach auf Approve, während tatsächlich niemand den Code reviewed.
    • Ich frage mich, wie groß das Team ist. Vermutlich skaliert dieser Ansatz nicht über fünf Engineers hinaus.
      Bei Problemen wie „das gesamte System, von dem ich abhängig war, ist verschwunden“ halte ich automatisierte Tests für sehr wichtig, die das auch dann erkennen können, wenn die Person, die von diesem System abhängt, gerade nicht dabei ist.
      Ich bin außerdem ein starker Befürworter von geteilter Verantwortung für alles. Es ist natürlich, dass Menschen bestimmte Teile der Codebase, besonders von ihnen erstellte Komponenten, faktisch besitzen, aber das führt zu Silos und einem niedrigen Bus-Faktor. Es sollte nicht dazu kommen, dass eine Person ein System besitzt und dieses System wiederum von einer einzelnen anderen Komponente abhängt.
    • Ich frage mich, was verhindert hätte, dass diese kaputte Änderung in Produktion gegangen wäre, wenn der Kollege nicht im Review gewesen wäre. Wenn Code Reviews wichtig sind, wo stehen dann Tests?
    • Manchmal erstelle ich auch für Code, den ich ohnehin sofort mergen werde, einen PR und tagge andere Entwickler. Der Zweck ist, einen leicht nachvollziehbaren Pfad bereitzustellen, was warum gemerged wurde, damit Leute nicht übersehen, was in der Codebase passiert.
    • Bei nicht trivialen Änderungen oder Aufräumarbeiten ist ein zweites Augenpaar immer gut. Aber ein Ansatz, bei dem „alle alles lesen“, lässt sich nicht auf großes N skalieren.
      Wenn es zu viel zu lesen gibt, kann niemand mehr folgen; deshalb delegiert man, schreibt Dokumentation und hält Übersichtssessions ab.

  • Ich habe Code Reviews immer am besten als das Tor verstanden, durch das Code vom Besitz des Autors in den Besitz des Teams oder Projekts übergeht.
    Der Code, den ich reviewe, ist nicht mehr dein Code, sondern wird bald unser Code sein. Wartbarkeit ist dabei natürlich ein großer Faktor.

    • Das ist wirklich eine beneidenswerte und luxuriöse Umgebung.
      Unser Team hat angefangen, KI zu nutzen, also habe ich auf einen simplen Modus umgestellt: Ich hinterlasse keine Kommentare mehr und entscheide nur noch binär, ob „das völlig verrückt ist oder ob es durchgehen kann“.
      Ich schone damit meine Zeit und meine mentale Gesundheit.

  • Dadurch werden Reviewer und Autor nur noch bequemer
    Der Zweck von Code-Reviews ist vielschichtig. Geht es um schwer wartbaren Code, mögliche Bugs, darum, ob es einfacher und sauberer geht, ob es zum Code-Style des Projekts passt, ob auch andere den Code verstehen, ob Junior-Teammitglieder onboarded werden, ob Designentscheidungen einem Sanity Check unterzogen werden – all das gehört dazu.
    Solche oberflächlichen Texte wirken meist eher wie eine Selbstrechtfertigung fauler Code-Reviewer.

    • Für Reviews gibt es eine eigene Checkliste, auf die man achten sollte
      Man sollte prüfen, ob die Funktionalität gemäß Issue oder PR-Beschreibung das Ziel erreicht, ob unnötiger Code wie übrig gebliebene Debug-Ausgaben oder private API-Keys vorhanden ist, und ob offensichtliche Mängel wie Memory Leaks, unbehandelte Edge Cases, Sicherheitslücken oder veraltete API-Aufrufe vorliegen.
      Man sollte auch prüfen, ob sich der Code verständlicher machen lässt, ob Abstraktionen hinzugefügt oder entfernt werden sollten, ob Variablen- und Methodennamen besser sein könnten und ob mehr oder weniger funktionaler Stil sinnvoll wäre.
      Ebenso sollte man prüfen, ob er mit der Codebase oder dem Style Guide konsistent ist, ob es offensichtliche Performance-Verbesserungen gibt – etwa ein Hash-Set statt einer Liste zu verwenden oder Lazy Evaluation anzuwenden – und ob die Tests ausreichend sind.
      Der Behauptung, dass unverständlicher Code nicht durchgewunken werden sollte, stimme ich auch nicht vollständig zu. Manche Codes sind einfach wirklich schwer zu verstehen; das Ziel ist, sie funktional korrekt und zugleich so verständlich wie möglich zu machen.
    • Die Branche hat sich intensiv bemüht, von „dem Autor die Schuld geben“ zu „dem Prozess und dem Team die Schuld geben“ überzugehen
      Dieser Text ist aber eher bloß Bait und vergleichbar damit zu sagen: „Die Leute denken, beim Abendessen gehe es darum, Essen zu essen, aber eigentlich geht es nicht ums Essen, sondern darum, mit Familie und Freunden in Verbindung zu treten.“ Das ist eine bestimmte Art von schlampiger reduktionistischer Argumentation, die auf HN gut ankommt.
    • Da KI das Schreiben und Ausliefern von Code inzwischen beschleunigt hat, sollte sich der Schwerpunkt auf Reviews verlagern. Wir müssen prüfen, ob der Code tatsächlich richtig läuft, ob all unsere Annahmen stimmen und ob es unbeabsichtigte Nebenwirkungen gibt.
      Ich hatte das Gefühl, dass Review und Debugging viel mehr Zeit kosten als das Schreiben und Produzieren von Code, und einfach „beten, dass es funktioniert“ geht nie gut aus.

  • Die Aussage „Durch Anschauen von Code findet man im Allgemeinen keine Bugs“ stimmt überhaupt nicht. Auf jeder Abstraktionsebene kann man durchaus welche finden, und so etwas nennt man Code Smell.
    Nicht geschlossene File Descriptors, nicht awaited Coroutines, riesige try/catch-Blöcke, die Fehler nicht protokollieren und irgendeinen Wert zurückgeben, falsche Type Casts usw. gehören alle dazu.
    Als allgemeines Prinzip sollte man Type Checker, Compiler und Runtime nicht bloß als Hürden betrachten, die man irgendwie passieren muss. Man sollte mit diesen Stufen zusammenarbeiten, anerkennen, dass sie wertvolle Werkzeuge sind, und nicht gegen sie arbeiten.

    • Ich weiß nicht, wovon hier die Rede ist. Ich habe schon Bugs allein durch Code-Review gefunden, ohne den Code auszuführen; umgekehrt haben andere das bei meinem Code getan, und ich habe solche Szenen auch in Reviews anderer Leute gesehen.
      Man kann „im Allgemeinen“ sicher irgendwie so definieren, dass es technisch wahr wird, aber dann verliert die Aussage ziemlich ihren Sinn.

  • Wenn man weitreichende Aussagen über Code-Reviews treffen will, ist es wichtig zu definieren, welche Art von Code-Review gemeint ist.
    Heute sind asynchrone PR-Reviews im GitHub-Stil mit Inline-Kommentaren der Standard, aber Reviews bestehen nicht nur daraus. Früher gab es auch persönliche Reviews mit Prozessen, die eher an eine Dissertationsprüfung oder einen Konferenzvortrag erinnerten.
    Die Literatur, die Code-Reviews als nützliche Qualitätspraktik beschreibt – genauer gesagt als eine der wenigen tatsächlich nützlichen Qualitätspraktiken –, stammt überwiegend aus deutlich stärker strukturierten Review-Prozessen als den heutigen.
    Persönlich empfand ich PR-Reviews im GitHub-Stil vor LLMs eher als etwas, das einem das Gefühl gibt, der Prozess sei in Ordnung, oder als Mittel, um Governance-Checkboxen abzuhaken; in der LLM-Ära scheint mir ihr Kosten-Nutzen-Verhältnis deutlich schlechter geworden zu sein, sodass sie wohl verschwinden werden.

    • Synchrone Reviews sind auch heute noch möglich. Einer meiner frühen Manager brachte mir bei: Wenn ein „Standard“-Code-Review mehr als einmal hin und her geht, ist es fast immer besser, sich persönlich zu treffen – oder, wenn auch nur eine Person remote ist, das per Zoom zu klären – und die Einigung anschließend als Kommentar festzuhalten.
      Um eine etwas erzwungene technische Analogie zu bemühen: Asynchrone Textkommunikation ist im Hinblick auf erfolgreich codierbare Information verlustreicher als Sprache und hat auch geringeren Durchsatz. Wenn man also viele Informationen austauschen muss, lohnt es sich, die Synchronisierungskosten zu zahlen.
    • In einem meiner ersten Jobs mussten wir Änderungspakete ausdrucken, reviewen und unterschreiben. Es gab sogar eine Person, die dafür zuständig war, die Endfassung in einem Aktenschrank aufzubewahren.
      Das war näher an traditionellem Engineering, und alle mussten Software als etwas Dauerhafteres betrachten.

  • Wartbarkeit sicherzustellen ist sicherlich einer der Vorteile von Code-Reviews, aber zu sagen, das sei ihr einziger Zweck, ist eine ziemlich gewagte Behauptung.
    Code-Reviews sind auch ein Werkzeug, mit dem das Team Codeänderungen nachvollziehen und gemeinsame Verantwortung für die gesamte Codebase übernehmen kann.

  • Code-Review ist nichts Einheitliches. Es gibt viele Gründe dafür: Wissensaustausch, Verantwortungswäsche, Codequalität, Compliance und so weiter; und wie immer hängt der jeweilige Zweck vom Nutzungskontext ab.

    • Zu sagen, die meisten Leute verstünden den Grund für Peer Reviews nicht, wirkt ziemlich ahnungslos. Schon der Glaube, es gebe nur einen einzigen Zweck, wirkt wie ein Zeichen dafür, dass jemand wenig Erfahrung mit der Arbeit in anderen Teams oder mit anderen Menschen hat.

  • Wenn der Autor Mathematiker ist, bedeutet „Durch Anschauen von Code findet man im Allgemeinen keine Bugs“ vermutlich nicht, dass es völlig unmöglich ist, Bugs zu finden.
    Gemeint ist eher, dass es nicht möglich ist, alle Bugs oder einen bestimmten Bug zu finden.

    • Nach meiner Erfahrung aus Mathevorlesungen an der Universität sind Mathematiker oft wirklich schlecht darin, mit anderen Menschen zu kommunizieren. Das erklärt, warum jemand glauben kann, seine Aussage bedeute etwas anderes als das, was fast alle daraus lesen.
    • Wenn das die Bedeutung ist, stimmt es.
      Um es mit dem Wartbarkeitsargument zu verbinden: Ein Ziel von Reviews ist auch, Code so einfach wie möglich zu machen, damit die Wahrscheinlichkeit steigt, dass er per Review debugbar ist. Im absoluten Sinn verhindert das keine Bugs, aber es erhöht die Wahrscheinlichkeit.
    • Der Autor, ein Mathematiker, scheint die Bedeutung seines natürlichsprachlichen Quantors nicht zu verstehen. „Durch Anschauen von Code findet man im Allgemeinen keine Bugs“ bedeutet „Durch Anschauen von Code findet man im Allgemeinen keinerlei Bugs“, nicht „Man kann nicht alle Bugs finden“.
      Die erste Interpretation ist relevant, aber falsch; die zweite ist wahr, aber nicht relevant.
      Wahrscheinlich wollte der Autor sagen: „Einen gegebenen Bug kann man im Allgemeinen nicht durch Anschauen des Codes finden“, also „Für jeden Bug B gilt nicht, dass man B finden kann“; aber auch das ist zwar wahr, geht jedoch am Kern vorbei.

  • Ich habe sowohl mit Leuten zusammengearbeitet, die PR-Vorschläge häufig ablehnen, als auch mit Leuten, die Vorschläge annehmen.
    Bei Menschen, die Vorschläge annehmen, frage ich mich, ob sie die Ownership bis zu einem gewissen Grad mit mir teilen wollen. Es fühlt sich so an, als würden wir beide den Code warten und verstehen und in dieselbe Richtung schauen.
    Umgekehrt sinkt meine Bereitschaft, mich an PRs von Leuten zu beteiligen, die PR-Vorschläge ablehnen. Wenn es am Ende ohnehin abgelehnt wird, warum sollte man dann Zeit investieren und gründlich reviewen?

    • In unserem Team setzen wir Kommentaren meist Präfixe wie thought, change, nit, fix oder chat voran.
      thought bedeutet zum Beispiel: „Vielleicht wird foo später häufiger, dann refactoren wir es“, change: „Das ist eine leaky abstraction; ich würde es gern wie bar modelliert sehen“, nit: „Der Name ist nicht ganz intuitiv, lass uns Baz oder Boo in Betracht ziehen“, fix: „Dieser Unit-Test prüft das falsche Feld“, und chat: „Das ist eine größere Entscheidung darüber, wie Lösungen dieser Kategorie künftig aussehen sollen; lass uns das zuerst im Team besprechen.“
      Manche Präfixe bedeuten, dass der PR bis zur Änderung blockiert ist, andere, dass es ein Kommentar ist, den man annehmen kann oder auch nicht. Für die Autorin oder den Autor wird klarer, was „wir müssen zu einem gemeinsamen Verständnis kommen“ ist und was eher „Ausdruck einer Präferenz“ oder „Beobachtung“ ist.
      Allerdings sollte man nicht gekränkt sein, wenn man ein nit hinterlässt und die andere Person nicht zustimmt und es ignoriert. Wenn man stark dabei empfunden hat, hätte es kein nit sein dürfen.
    • Wenn man etwas für wichtig hält, sollte man einen blockierenden Vorschlag hinterlassen und das Gespräch erzwingen.