Peer / Code Review Frustrationen

Ich würde mich nicht als Superstar-Entwickler bezeichnen, sondern als relativ erfahren. Ich versuche, die Codequalität auf einem hohen Niveau zu halten, und bin immer bemüht, meinen Codierungsstil zu verbessern, Code effizient, lesbar und konsistent zu machen sowie das Team zu ermutigen, Muster und Methoden zu befolgen, um die Konsistenz sicherzustellen. Ich verstehe auch die Notwendigkeit eines Gleichgewichts zwischen Qualität und Geschwindigkeit.

Um dies zu erreichen, habe ich meinem Team das Konzept der Begutachtung vorgestellt. Zwei Daumen hoch in Github-Pull-Request für einen Merge. Super - aber meiner Meinung nach nicht ohne Schluckauf.

Ich sehe oft Peer-Review-Kommentare von denselben Kollegen wie -

• Wäre gut ein Leerzeichen nach hinzuzufügen <INSERT SOMETHING HERE>
• Unerwünschte zusätzliche Linie zwischen Methoden
• Punkt sollte am Ende der Kommentare in docblocks verwendet werden.

Aus meiner Sicht betrachtet der Rezensent die Code-Ästhetik nur oberflächlich und führt nicht wirklich eine Code-Rezension durch. Die Überprüfung des Kosmetikcodes kommt mir als arrogante / elitäre Mentalität vor. Es mangelt ihm an Substanz, aber man kann nicht wirklich zu viel damit streiten, weil der Rezensent technisch korrekt ist . Ich würde viel lieber weniger der oben genannten Arten von Bewertungen und mehr Bewertungen wie folgt sehen:

• Sie können die zyklomatische Komplexität reduzieren, indem Sie ...
• Beende früh und vermeide if / else
• Abstrahieren Sie Ihre DB-Abfrage in ein Repository
• Diese Logik gehört nicht wirklich hierher
• Wiederholen Sie sich nicht - abstrakt und wiederverwenden
• Was würde passieren, wenn Xes als Argument an method übergeben würde Y?
• Wo ist der Komponententest dafür?

Ich finde, dass es immer die gleichen Arten von Leuten sind, die die kosmetischen Arten von Bewertungen abgeben, und die gleichen Arten von Leuten, die meiner Meinung nach die Peer-Bewertungen auf der Grundlage von "Qualität und Logik" abgeben.

Was ist (wenn überhaupt) der richtige Ansatz für die Begutachtung durch Fachkollegen? Und habe ich Recht, wenn ich mit denselben Leuten frustriert bin, die im Grunde genommen den Code überfliegen, um nach Rechtschreibfehlern und ästhetischen Fehlern zu suchen, anstatt nach tatsächlichen Codefehlern?

Wenn ich richtig liege, wie würde ich Kollegen dazu ermutigen, tatsächlich nach Fehlern im Code zu suchen, und dabei kosmetische Korrekturen vorschlagen?

Wenn ich falsch liege - bitte klären Sie mich auf. Gibt es Faustregeln für eine gute Codeüberprüfung? Habe ich den Punkt verpasst, von dem Code-Bewertungen sind?

Aus meiner Sicht geht es bei der Codeüberprüfung um die gemeinsame Verantwortung für den Code. Ich würde mich nicht wohl fühlen, wenn ich dem Code die Daumen hoch geben würde, ohne Logik, Lesbarkeit und Funktionalität zu adressieren / zu überprüfen. Ich würde mich auch nicht darum kümmern, eine Zusammenführung für ein solides Stück Code zu blockieren, wenn ich bemerken würde, dass jemand einen Punkt in einem Dokumentblock weggelassen hätte.

Wenn ich den Code überprüfe, verbringe ich möglicherweise zwischen 15 und 45 Minuten pro 500 Lok. Ich kann mir nicht vorstellen, dass diese flachen Reviews länger als 10 Minuten dauern, wenn das die Tiefe der Reviews ist, die sie durchführen. Wie viel Wert hat der Daumen nach oben vom flachen Prüfer? Dies bedeutet sicherlich, dass nicht alle Daumen gleich schwer sind und möglicherweise ein Überprüfungsprozess mit zwei Durchgängen durchgeführt werden muss. Ein Daumen für tiefe Reviews und ein zweiter Daumen für das "Polieren"?

Arten von Bewertungen

Es gibt keine echte Möglichkeit, Peer Reviews durchzuführen. Es gibt viele Möglichkeiten zu beurteilen, ob der Code eine ausreichend hohe Qualität aufweist. Offensichtlich stellt sich die Frage, ob es sich um einen Buggy handelt oder ob es Lösungen gibt, die nicht skalieren oder spröde sind. Konformitätsprobleme mit lokalen Standards und Richtlinien sind zwar möglicherweise nicht so kritisch wie bei einigen anderen, tragen aber auch zu einem hohen Qualitätsstandard bei.

Arten von Gutachtern

So wie wir unterschiedliche Kriterien für die Beurteilung von Software haben, sind auch die Personen, die die Beurteilung vornehmen, unterschiedlich. Wir haben alle unsere eigenen Fähigkeiten und Vorlieben. Einige denken möglicherweise, dass die Einhaltung lokaler Standards sehr wichtig ist, genauso wie andere sich mehr mit der Speichernutzung oder der Codeabdeckung Ihrer Tests befassen und so weiter. Sie möchten alle diese Arten von Überprüfungen, da sie Ihnen als Ganzes dabei helfen, besseren Code zu schreiben.

Ein Peer Review ist eine Zusammenarbeit, kein Markenspiel

Ich bin mir nicht sicher, ob Sie das Recht haben, ihnen zu sagen, wie sie ihre Arbeit tun sollen. Wenn Sie mit Sicherheit nichts anderes wissen, gehen Sie davon aus, dass diese Person versucht, ihren Beitrag so zu leisten, wie sie es für richtig hält. Wenn Sie jedoch Verbesserungspotenzial sehen oder vermuten, dass sie nicht verstehen, was in einem Peer Review erwartet wird, sprechen Sie mit ihnen .

Bei einem Peer Review geht es darum, Ihre Kollegen einzubeziehen . Die Beteiligung wirft keinen Code über eine Mauer und wartet darauf, dass eine Antwort zurückgeworfen wird. Engagement arbeitet zusammen, um besseren Code zu erstellen. Unterhalten Sie sich mit ihnen.

Rat

Gegen Ende Ihrer Frage haben Sie geschrieben:

Wie kann ich Kollegen dazu ermutigen, tatsächlich nach Fehlern im Code zu suchen, die mit offensichtlichen ästhetischen Fehlern in Einklang stehen?

Wieder ist die Antwort Kommunikation. Vielleicht können Sie sie fragen: "Hey, ich weiß es zu schätzen, dass Sie diese Fehler aufgegriffen haben. Es würde mir sehr helfen, wenn Sie sich auch auf einige tiefere Themen konzentrieren könnten, z Hilfe."

In einer pragmatischeren Hinsicht teile ich die Kommentare zur Codeüberprüfung persönlich in zwei Lager auf und formuliere sie entsprechend: Dinge, die behoben werden müssen, und Dinge, die kosmetischer sind. Ich würde niemals verhindern, dass fester, funktionierender Code eingecheckt wird, wenn am Ende einer Datei zu viele Leerzeilen stehen. Ich werde jedoch darauf hinweisen, aber ich werde es mit etwas wie "unseren Richtlinien tun", dass am Ende eine einzelne Leerzeile steht und Sie 20 haben. Es ist kein Show-Stopper, aber wenn Sie eine Chance haben, haben Sie Vielleicht möchten Sie es beheben ".

Hier ist noch etwas zu beachten: Es mag ein kleiner Witz von Ihnen sein, dass sie eine so flache Überprüfung Ihres Codes durchführen. Es kann sehr gut sein , dass ein Haustier ärgert von ihnen ist , dass Sie (oder ein anderer Mitspieler, der eine ähnliche Bewertung wird) in Bezug auf Ihre eigene Organisation Coding - Standards schlampig sind, und dies ist , wie sie gewählt haben, dass mit Ihnen zu kommunizieren.

Was ist nach der Überprüfung zu tun

Und zum Schluss noch ein kleiner Ratschlag nach der Überprüfung: Wenn Sie nach einer Überprüfung Code schreiben, sollten Sie in Betracht ziehen, sich um alle kosmetischen Dinge in einem Commit und die funktionalen Änderungen in einem anderen zu kümmern. Das Mischen der beiden kann es schwierig machen, signifikante Änderungen von unwesentlichen zu unterscheiden. Nehmen Sie alle kosmetischen Änderungen vor und bestätigen Sie diese mit der Meldung "kosmetisch, keine funktionalen Änderungen".

Die Leute kommentieren Code-Formatierungen und Tippfehler, weil sie leicht zu erkennen sind und nicht viel Aufwand erfordern.

Dieser Teil ist einfach zu reparieren - fast jede Sprache hat ein Linter- oder Style-Checker-Tool. Stecken Sie es in Ihren Build-Prozess, damit der Build fehlschlägt, wenn redundanter Speicherplatz vorhanden ist. Infolgedessen gibt es keine Stilprobleme, die kommentiert werden müssen.

Es könnte eine ziemliche Herausforderung sein, sie dazu zu bringen, echte Probleme zu finden. Dies erfordert nicht nur eine besondere neugierige und detailorientierte Denkweise, sondern auch eine erhebliche Motivation.

Und diese Motivation kommt aus Erfahrung. Erfahrung mit dem Versuch, mit beschissenem Code zu arbeiten, der von früheren Entwicklern geschrieben wurde. Ältere Ingenieure haben viel davon. Das Schwimmen im Meer der Scheiße macht einen ziemlichen Wunsch, nicht wieder dorthin zu gelangen.

Wenn ich eine Hauptsache auswählen muss, um sie während der Codeüberprüfung zu überprüfen, dann ist das die Wartbarkeit des Codes. Entwickler, die diesen Code lesen, sollten ihn jederzeit verstehen und bereit sein, ihn zu verbessern und zu ändern. Wenn er keine Ahnung hat, was hier vor sich geht, muss er es sofort mitteilen und der Code muss neu geschrieben werden.

Hier kommt die praktische Antwort:

Szenario 1 : Sie sind das leitende Mitglied und jemand, der die Praxis bestimmen kann

Rufen Sie ein Meeting an und legen Sie die Regeln und Richtlinien für die Codeüberprüfung fest. Vertrauen Sie mir, klare Richtlinien funktionieren besser als jedes Ehrensystem oder Training. Stellen Sie klar, dass Probleme mit der Code-Formatierung überhaupt nicht auftreten dürfen. Einige Mitglieder werden Einwände erheben. Hören Sie ihnen zu und bitten Sie sie, die Richtlinien für die ersten Monate als Experiment zu befolgen. Zeigen Sie die Bereitschaft zur Änderung, wenn die aktuellen Richtlinien nicht funktionieren.

Szenario 2 : Sie sind keine Person, die über das Training entscheidet, oder Sie sind ein relativ junges Mitglied im Team

Am besten lösen Sie die Probleme mit der Codeüberprüfung, bis Sie Szenario 1 erreicht haben .

Die einfache Antwort, um "triviale" Code-Überprüfungskommentare zu vermeiden, besteht darin, (aus Gründen der Effizienz) darauf zu bestehen, dass der Überprüfer derjenige ist, der sie repariert. Wenn der Prüfer feststellt, dass Sie (Horror!) Einen Punkt verpasst oder einen Kommentar falsch geschrieben haben, sollte er ihn einfach korrigieren und weitermachen.

Nach meiner Erfahrung bedeutet dies:

• Ihre Rezensenten machen diese relativ sinnlosen Rezensionskommentare nicht mehr und geben sie nicht mehr weiter, um sie zu reparieren.
• Nur die meisten OCD-Rezensenten werden sie reparieren, was bedeutet, dass die meisten Ihrer Bewertungen nicht bestanden werden. Dies hat zur Folge, dass Entwickler zu umfangreicheren Überprüfungen aufgefordert werden. Diejenigen, die Wissenswertes melden, weil sie den Code nicht tatsächlich überprüfen, müssen am Ende begründen, warum alle ihre Überprüfungen kommentarlos verlaufen.

In jedem Fall ist dies ein Vorteil. Die Kosten einer fehlgeschlagenen Überprüfung sind hoch, da ein Entwickler damit aufhören muss, an was er gerade arbeitet, und seinen Code und die nachfolgende Nachprüfung überprüfen muss. Wenn eine Überprüfung echte Codequalität oder Architekturprobleme feststellt, sind diese Kosten jeden Cent wert, die Kosten für Kleinigkeiten jedoch nicht.

Überprüfen Sie den Überprüfungsprozess

Zusätzlich zu den Code-Überprüfungen würde ich vorschlagen, den Überprüfungsprozess regelmäßig zu überprüfen. Es gibt sicherlich auch hier eine Menge zu lernen, z. B. wie man richtige Code-Reviews gibt.

Gewöhnlich sind einige der Fahrrad-Shedder nur unerfahren und wissen einfach nicht, wonach sie suchen sollen. Sie brauchen ein wenig Anleitung nicht nur in Bezug auf ihren Code, sondern auch im Hinblick auf hilfreiche Codeüberprüfungen. Das Bereitstellen von Feedback zu den Bewertungen führt zu einem Lernprozess, der (hoffentlich) zu besseren Bewertungen und besserem Code führt.

Als nächstes könnte es eine gute Idee sein, eine Reihe von Werten und Kriterien (lose) zu formalisieren, was die Organisation oder das Team als guten Code ™ empfindet und welche Anti-Muster vermieden werden sollten. Es geht nicht darum, etw. Einzustellen. konkret, aber von Anfang an einen gemeinsamen Konsens über die Codequalität zu erzielen .

Als jemand, der auf beiden Seiten davon war (Code, der von anderen geschrieben wurde, überprüfen zu lassen sowie Code, den ich geschrieben habe), würde ich sagen, dass ich drei Kategorien habe, in die jedes Feedback fällt. Nun, vier, es gibt auch den entzückenden Fall von "alles ist gut".

"Wäre nett, würde dich aber nicht blockieren" (Wenn alle Rückmeldungen in diese Kategorie fallen, sende ich die Zusammenführungsanforderung möglicherweise mit einem "wird bei XX: XX zusammengeführt, es sei denn, du sagst mir, dass du sie reparieren möchtest". , oder "sicher, gehen Sie vor, um einzuchecken, welcher Block, den das System werfen würde, deaktiviert wurde"):

• Punkt am Ende von Sätzen vergessen (in Dokumentzeichenfolgen oder Kommentaren). Unbeholfene Grammatik in allem, was nicht in irgendeiner Form oder Form an Benutzer ausgegeben wird (wiederum Dokumentzeichenfolgen und Kommentare)
• Code, der eleganter aussieht, aber verständlich und idiomatisch ist (ich werde wahrscheinlich sogar meinen Vorschlag einbringen, sodass es einfach ist, ihn zu verlassen oder zu korrigieren)

"Dinge, die repariert werden müssen, aber ich vertraue darauf, dass Sie das tun" (wenn alle Dinge in diese oder die vorherige Kategorie fallen, antworte ich mit "diese reparieren", verschmelze ich, wenn Sie mir sagen, dass Sie re done "(und an diesem Punkt werde ich wahrscheinlich schnell scannen, um festzustellen, dass nichts anderes aufgetaucht ist)):

• Kleinere Probleme ("Sie vergleichen einen Booleschen mit true, das ist ein bisschen paranoid ...", ...)
• Kleinere Verstöße gegen den Stil ("Die Stilrichtlinie sagt X, was Sie getan haben, liegt außerhalb dessen. Ich würde Y oder Z tun, je nachdem, welchen Weg Sie einschlagen möchten.")
• Ein paar fehlende Testfälle, die nicht schwer zu schreiben sein sollten

Und schließlich: "Dinge, die problematisch sind, muss ich Ihren Code erneut überprüfen, sobald Sie diese Probleme behoben haben." es gibt auch ein paar kleinere Dinge, es wäre schön, einige davon zu sehen, während Sie dabei sind ", wenn etwas aus den ersten beiden Kategorien vorhanden ist):

• Das wären Dinge wie "Nein, es gibt wirklich eine VIEL bessere Schreibweise", "Sie berechnen nicht, was Sie erwarten, weil Ihr Komponententest diese Randfälle verfehlt" und alle anderen schwerwiegenden Probleme.

Anscheinend haben einige Leute die wichtigste Frage vergessen: Was möchten Sie mit Code-Überprüfungen tatsächlich erreichen?

Der Zweck der Codeüberprüfung ist es, fehlerfreien und wartbaren Code schneller zu erhalten. Code-Überprüfungen erreichen dies auf verschiedene Arten: Entwickler schreiben in erster Linie besseren Code, weil sie wissen, dass er überprüft wird, das Wissen im Rahmen des Überprüfungsprozesses geteilt wird und Fehler gefunden werden, weil der Prüfer nicht blind für ihre eigenen Fehler als Entwickler ist kann sein.

Wenn Sie den Überprüfungsprozess als Chance sehen, Ihre Kollegen zu entlassen oder Arbeit für sie zu schaffen, dann machen Sie es falsch.