Peer / Code Review Frustrationen

27

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"?

Soße
quelle
2
Haben Sie versucht, dies dieser Person gegenüber zu erwähnen?
Bryan Oakley
11
Ich hatte einen Vorgesetzten, der in allen Kommentaren Punkte benötigte, sowie eine korrekte Groß- und Kleinschreibung, Zeichensetzung und Grammatik. Er war auch sehr anal in Bezug auf Leerzeichen. Es hat mich verrückt gemacht, aber es hat auch zu sehr lesbarem, konsistentem Code geführt.
Bryan Oakley
4
Die ersten drei Aufzählungszeichen in Ihrem Post sind Fahrradabwürfe, es sei denn, sie sind Teil eines Standardgeschäfts mit Codierungsstil. Wenn Sie Dokumentation schreiben, würde ich perfekte Rechtschreibung und Grammatik erwarten. Angesichts der Fülle von Rechtschreib- und Grammatikprüfungen in Textverarbeitungsprogrammen ist dies heutzutage nicht schwer zu erreichen. Ebenso können Probleme mit dem Codestil häufig in entsprechend intelligenten Code-Editoren automatisiert werden. Ich würde nicht erwarten, solche Dinge in einer Codeüberprüfung zu sehen. Die Zeit eines jeden ist zu wertvoll.
Robert Harvey
2
Die arrogante / elitäre Überprüfung ist einfach und erfordert fast keinen Aufwand. Um eine technische Überprüfung durchzuführen, müssen Sie den Code lesen und verstehen und dann über eine bessere Lösung nachdenken. Das bedeutet, dass Sie arbeiten müssen. Das Ergebnis Ihres Vorschlags überrascht mich nicht. Sie sollten den Überprüfungsprozess mit messbaren und genau definierten Aufgaben organisieren, um etwas zu erreichen.
JoulinRouge
2
Wenn Sie Agile verwenden, können Sie dies bei Retros ansprechen, ohne auf ein einzelnes Ziel hinzuweisen. Erwähnen Sie, dass die Hauptfunktion der Codeüberprüfung die Korrektheit des Codes und nicht die Ästhetik des Codes ist. In diesem Fall wird es zu einem "Bedürfnis nach Veränderung" und etwas, das Sie kontinuierlich ansprechen können, bis es sich tatsächlich ändert.
Canadian Coder

Antworten:

22

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".

Bryan Oakley
quelle
Vielen Dank für eine hervorragende Antwort. Ich denke, meine Frustrationen liegen darin, dass biggerProbleme nicht angesprochen werden. B. fehlende Indizes in einer DB-Tabelle. Oder Sie versuchen, eine Methodik oder einen Algorithmus zu verwenden, ohne die Argumentation zu verstehen und dies als solche falsch zu tun. Für mich - diese sind wichtiger und sollten in erster Linie angesprochen und gelöst werden - wobei die Ästhetik ein zweitrangiges Anliegen ist.
Soße
1
Wissen Sie, dass größere Probleme übersehen werden, oder haben Sie nur Angst davor? Wenn ein großes Problem übersehen wird, können Sie in einer Retrospektive oder einer Teambesprechung darauf hinweisen, dass dies die Art von Dingen sind, die bei der Codeüberprüfung berücksichtigt werden sollten. Sie könnten sogar in Betracht ziehen, zu fragen, wer im Team sich auf Datenbankänderungen konzentriert. Wenn niemand die Hand hebt, ernennen Sie möglicherweise einen Mitarbeiter, der versucht, sich bei der nächsten Überprüfung nur auf Datenbankänderungen zu konzentrieren.
Bryan Oakley
Um ehrlich zu sein - die meisten kosmetischen Kommentare richten sich in der Regel nicht an meinen Code. Wenn ich den Code anderer überprüfe, sehe ich Kommentare gegen PRs in Bezug auf Kosmetika, direkt neben Code, den ich als großes Problem betrachten würde. Darüber hinaus ist ein Großteil der Muttersprache des Teams nicht Englisch. Aus meiner Sicht ist der seltsame Rechtschreib- / Grammatikfehler also verzeihbar. Ich bin nicht hier, um ihre Verwendung von Englisch in einem Dokumentblockkommentar zu überprüfen, sondern ihren Code.
Soße
2
Nun, ihre Kommentare sind Teil des Quellcodes, und wenn sie unnötig schwer zu analysieren, irreführend oder sogar falsch sind, kann es für jeden, der später Gelegenheit hat, sich diesen Code aus irgendeinem Grund anzusehen, ein schlechter Service sein. Sie müssen nicht formal oder kunstvoll sein, um ihr Ziel zu erreichen, aber gebrochenes Englisch behindert das Ziel, ungeachtet der Beherrschung der Sprache durch die Autoren. Besser keine Kommentare als schlechte.
Deduplizierer
1
Die meisten Menschen wissen es zu schätzen, wenn ihnen auf Fehler in der Sprache hingewiesen wird, damit sie sich verbessern können.
gnasher729
7

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.

Alex
quelle
Ich stimme Ihnen in Ihren Kommentaren zu. Und wenn Sie ocean of shitdas von mir geschrieben sehen - dann würde ich Sie ermutigen, vorzuschlagen, dass ich umschreibe. Aber wenn du das ignorierst, shitaber mich gebeten hast, das kosmetische Zeug zu reparieren - das ist es, was mich frustriert.
Soße
4

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 .

Kshitij Upadhyay
quelle
2

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.

gbjbaanb
quelle
0

Ü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 .

JensG
quelle
0

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.
Vatine
quelle
0

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.

gnasher729
quelle