Was ist der beste Weg für einen Programmierer, mit Code-Überprüfungen umzugehen?

16

Ich bin ziemlich neu in der Codeüberprüfung, aber ich habe während meiner Promotion jahrelang programmiert - was Sie nicht immer zu einem guten Programmierer macht!

Was tun Sie, wenn der Prüfer Ihren Code ändert und Sie Zeile für Zeile durchläuft, wenn Sie nicht einverstanden sind? Manchmal haben Sie die Auswahl X getroffen und der Prüfer hat sie in Y geändert, und Sie hatten Y im Kopf, haben sich jedoch entschieden, dies aufgrund der Nachteile nicht zu tun. Der Prüfer macht jedoch geltend, dass diese Nachteile nicht wichtig sind. Sollten Sie Ihre Meinungsverschiedenheit verbalisieren oder einfach nicht und auf sie hören?

Es ist schwierig, wenn Sie Kritik nicht gut akzeptieren und wenn der Prüfer eine höhere Position in der Organisation einnimmt. Es wäre nicht gut, als zu defensiv zu wirken.

Paul Richards
quelle
1
Habe
@gnat: Diese Frage ist eindeutig kein Duplikat. Schauen Sie sich die am besten bewertete, akzeptierte Antwort auf diese Frage an. Wenn es hier als Antwort gegeben worden wäre, wäre es abgelehnt worden, weil es diese Frage nicht beantwortet .
Michael Shaw
@gnat: Bei der anderen Frage geht es darum, wie der Code eines anderen in einer Überprüfung abgelehnt wird, und bei dieser Frage geht es darum, wie die Kritik am eigenen Code in einer Überprüfung behandelt wird. Die einzige Ähnlichkeit, die ich sehen kann, ist, dass beide Codeüberprüfungen beinhalten.
Michael Shaw

Antworten:

19

Es ist wahr, dass es nicht hilfreich ist, als defensiv zu wirken. Aber dann wird auch nichts kritisiert. Wenn Sie also das Gefühl haben, dass dies geschieht, sollten Sie Ihre Bedenken zum Ausdruck bringen, dass die Codeüberprüfung Ihnen nicht dabei hilft, besseren Code in der Organisation zu schreiben.

Der Prüfer ist dafür verantwortlich, den Code auf tatsächliche Verstöße und Mängel zu überprüfen und ihn nicht dazu zu verwenden, Ihren Code so zu schreiben, wie er es getan hätte. Dies bedeutet, dass die Überprüfung dazu dient, zu überprüfen, wie Sie etwas getan haben, und auf Bereiche hinzuweisen, in denen Sie einen Fehler begangen haben (etwas, was die Besten von uns tun) oder das Framework oder die Standards oder "historische Gründe" nicht verstanden haben So ist es, wo du bist.

Wenn es also mehrere Möglichkeiten gibt, etwas zu tun, muss es einen guten Grund geben, warum Ihr Code auf eine alternative Weise geändert wird, andernfalls ist es einfach eine nicht konstruktive Abwandlung, die Ihnen nicht hilft.

Denken Sie auch daran, dass der Rezensent auch nicht perfekt ist. Möglicherweise haben sie eine Idee, dass Y der richtige Weg ist, und sie haben nicht erkannt, dass X besser ist. Sie sollten die Gründe, warum Sie es getan haben, auf die Art und Weise X erklären. Der Prüfer kann Ihnen zustimmen oder Ihnen sagen, warum Y eine bessere Lösung ist - es kann andere Faktoren geben, von denen Sie nicht wissen, dass er dies tut.

Kurz gesagt, Bewertungen sind eine Möglichkeit, Teammitglieder dazu zu bringen, über ihre Codeänderungen zu kommunizieren. Sprechen Sie mit dem Rezensenten darüber.

Wenn es hilft, sprechen Sie unparteiisch, als wären Sie nicht einmal der Autor des zu überprüfenden Codes. Der Code gehört schließlich der Firma oder dem Team, und das Team muss ihn pflegen. Du warst zufällig derjenige, der es geschrieben hat, das ist nicht so wichtig, wie viele glauben.

gbjbaanb
quelle
7
"Der Prüfer ist dafür verantwortlich, den Code auf tatsächliche Verstöße und Mängel zu überprüfen und ihn nicht dazu zu verwenden, Ihren Code so zu schreiben, wie er es getan hätte.": +1 für diesen Hinweis.
Giorgio
+1 "Mit Bewertungen können Teammitglieder über ihre Codeänderungen
informiert werden
20

Das Schreiben von Computercode ist ein hervorragendes Beispiel für das Treffen von Entscheidungen unter Unsicherheit . Es gibt immer widersprüchliche Belastungen, und wie Sie in einer bestimmten Frage entscheiden, hängt davon ab, welche Belastungen Sie wahrnehmen und wie hoch Sie sie einschätzen.

Wenn ein Prüfer mit Ihrer Entscheidung nicht einverstanden ist, bedeutet dies, dass er andere Belastungen / Risiken sieht als Sie oder einige von ihnen für größer / kleiner hält als Sie. Sie müssen unbedingt über diese Unterschiede sprechen, denn wenn Sie dies nicht tun, bleiben die Probleme bestehen, die überhaupt zu Meinungsverschiedenheiten geführt haben.

Wenn Ihr Prüfer älter ist, kann seine Erfahrung zu Recht darauf hinweisen, dass dieses oder jenes Risiko in der Praxis nicht sehr relevant ist, oder dass eine Art von Fehler in Ihrer Organisation schon seit langem auftritt und es sich lohnt, besonders vorsichtig zu sein es zu vermeiden. Umgekehrt müssen Sie dies unbedingt ausdrücken, wenn Sie das Gefühl haben, etwas zu wissen, das Ihr Prüfer wahrscheinlich nicht weiß. Schweigen bedeutet für Sie eine Pflichtverletzung.

Der wichtigste Teil des Umgangs mit der Situation besteht darin zu verstehen, dass Kritik an Designentscheidungen praktisch immer keine Kritik an der Persönlichkeit eines Menschen ist. (In den seltenen Fällen, in denen dies tatsächlich der Fall ist, werden Sie dies früh genug bemerken, und wenn Sie jemandem wirklich nicht gefallen können, macht nichts, was Sie tun, einen Unterschied. Wo liegt also der Schaden, wenn Sie Best Practices befolgen? sobald wie möglich.) Es ist nur eine Folge davon, dass unterschiedliche Menschen die vielen Risiken und Chancen, die mit Computercode verbunden sind, unterschiedlich wahrnehmen und angesichts der Komplexität des modernen Computercodes nur zu erwarten sind. Wenn Sie über die Unterschiede sprechen, können Sie den Code verbessern und Ihre Zusammenarbeit in diesem Fall und in zukünftigen Fällen verbessern.

Kilian Foth
quelle
4

Die anderen Antworten enthalten bereits sehr gute Informationen. Ich möchte ein wenig auf einige Aspekte eingehen, die von gbjbaanb angedeutet wurden (siehe meinen Kommentar zu seiner Antwort).

Meiner Erfahrung nach habe ich bei Code-Überprüfungen verschiedene Arten von Rückmeldungen beobachtet:

  1. "Ich glaube ehrlich, dass dies falsch / suboptimal ist und dass Sie es auf diese Weise ändern sollten." Normalerweise nehme ich diese Art von Feedback ernst und ich werde mich der vorgeschlagenen Änderung nur widersetzen, wenn ich der Meinung bin, dass ich eine starke Position dagegen habe.
  2. "Ich finde Ihre Lösung in Ordnung, überlegen Sie sich diese Alternative, aber es liegt an Ihnen, ob Sie die Änderung akzeptieren oder nicht." Ich nehme diese Art von Feedback auch ernst: Der Rezensent schlägt eine Alternative vor, obwohl er nicht der festen Meinung ist, dass seine Lösung besser ist. Ich habe die Möglichkeit, etwas zu lernen, und ich werde die Änderung akzeptieren, wenn es mir besser gefällt. Ansonsten findet es der Rezensent in Ordnung, wenn ich den Code nach meinem Geschmack behalte.
  3. "Ich hätte es anders gemacht, also musst du es ändern, Punkt." Oder sogar "Oh, ich habe deinen Code geändert, weil ...", die Änderung wurde nicht vorgeschlagen, sie wurde bereits festgeschrieben! Es wird eine kurze Erklärung zu der Änderung gegeben, mit dem Hinweis, dass nicht viel Zeit bleibt, um die Details zu besprechen, da wir mit der nächsten Aufgabe fortfahren müssen.
  4. Der Rezensent schlägt geringfügige Änderungen vor, die den Code nicht verbessern, sondern nur verändern. Wenn der Prüfer gebeten wird, die vorgeschlagenen Änderungen zu besprechen, führt er lange Diskussionen über triviale Details, bis Sie aus Erschöpfung aufgeben.

Die Optionen 3, 4 können als 1 und 2 getarnt sein: "Es war wirklich wichtig, dies so zu tun, wie ich es vorgeschlagen habe." oder "Sie können die Änderung ablehnen, wenn Sie wirklich wollen.", sagte mit einem Ton, der in der Tat genau das Gegenteil von dem bedeutet, was gesagt wird. Für den Fall, dass Sie Änderungen ablehnen, die Sie für unnötig halten, kann der gemeinsame Besitz von Code als Begründung für diese Einstellung verwendet werden: "Der Code gehört nicht Ihnen, er gehört dem Team!" Sie können erkennen, wenn die Absicht des Rezensenten nicht ehrlich ist, wenn der Rezensent nicht sehr offen für Diskussionen ist, verärgert ist und versucht, seine Lösung um jeden Preis voranzutreiben. Grundsätzlich haben sie sich bereits entschieden, und jede weitere Diskussion ist nur Zeitverschwendung.

IMO-Optionen 1 und 2 sind ein Zeichen für einen ehrlichen Gutachter, der versucht zu helfen, Ihnen etwas beizubringen und dabei Ihre Arbeit zu respektieren, und der auch offen ist, selbst etwas zu lernen. In diesem Szenario versuche ich, nicht zu stolz zu sein, wenn ich konstruktives Feedback von einem Rezensenten erhalte.

Die Optionen 3 und 4 lassen darauf schließen, dass ein Machtspiel im Gange ist: Wichtig ist, ob wir es auf meine oder Ihre Weise tun, und nicht, dass wir versuchen, eine gute Lösung zu finden, die beide zufriedenstellt. Dies kann mit dem Ego des Rezensenten zusammenhängen, aber auch damit, dass er keinen Code verstehen kann, der nicht seiner Denkweise entspricht. Sie werden normalerweise durch Code gestört, der ihnen nicht vertraut erscheint, und möchten der gesamten Codebasis ihren Weg aufzwingen.

Wenn die Situationen 3 und 4 zu oft vorkommen, kann die Teamarbeit ziemlich unangenehm werden. In einer solchen Situation würde ich erwägen, das Team zu verlassen.

Giorgio
quelle
Ich habe festgestellt, dass, wenn ich jemals das Gefühl habe, als 3 oder 4 rüberzukommen, ich entweder beweisen muss, was sie tatsächlich tun, ist gebrochen ("Sehen Sie, dies schlägt tatsächlich fehl, wenn der Nachname des Kunden Null ist") oder schreiben muss Überprüfen Sie, ob die vorgeschlagene Lösung tatsächlich die Änderung wert ist (möglicherweise ist mein Code besser lesbar, aber langsamer, oder umgekehrt. In diesen Fällen stört es mich normalerweise nicht, die Änderung zu überprüfen, es sei denn, der Unterschied ist erheblich (auch nicht) in Lesbarkeit oder Geschwindigkeit)).
Scragar
@scragar: Richtig: Man versucht immer, sich an die Fakten zu halten. Es kann jedoch manchmal lästig sein. Beispiel (im Kontext eines ziemlich umfangreichen Commits): Sie müssen einen std :: string mit einem QString vergleichen. Ihre Lösung: Konvertieren Sie std :: string in QString und vergleichen Sie mit der QString-Methode. Reviewer-Änderung: QString in std :: string konvertieren und mit der std :: string-Methode vergleichen. Sie können über unendliche Variationen nachdenken, wie Sie Code in äquivalenten Code umwandeln können, um zu zeigen, dass Sie dort waren. Es ist sehr wichtig, zwischen konstruktivem Feedback und Nitpicking zu unterscheiden.
Giorgio
3

Um Ihre Frage zu klären, was zu tun ist, wenn Sie nicht einverstanden sind ...

Reden wir sind der richtige Weg, wie die meisten Leute bereits betont haben.

Wenn Sie das bereits getan haben, vielleicht sogar mehr als einmal, ist eine nützliche Technik, die wir anwenden, wenn es in einigen Bereichen immer noch Uneinigkeit gibt, zu sagen: "Ja, es wäre gut, das zu überarbeiten."

Können wir dafür ein separates Ticket einreichen?

Mit einem separaten Ticket können Sie den Code anstelle des gefürchteten Modus "Abwandern und niemals weitermachen" abrufen, den ich an einigen Stellen gut gekannt habe. Das passt gut zu Agilität, macht die kleinstmögliche Menge und verteilt die Ladung. Manchmal bewirbt sich Yagni. Manchmal entscheidet der Produktmanager, dass in Bezug auf den Wert für den Kunden, die Fristen und die Ressourcen dringendere Anforderungen als dieser Refaktor vorliegen.

Michael Durrant
quelle
1

Die Codeüberprüfung ist wahrscheinlich im Allgemeinen eine gute Sache, aber meiner Erfahrung nach ist sie das beste Werkzeug für Entwickler in neuen Teams, die neue Codierungsrichtlinien verwenden oder wichtige Fehler beheben, damit das Risiko von Fehlern verringert wird. Wenn Sie glauben, dass Sie es besser wissen als der Prüfer, sollten Sie sich fragen, warum die von ihm vorgeschlagene Lösung besser ist und argumentieren Sie mit Ihren Einsichten in dieser Angelegenheit. Niemand weiß alles am besten, daher sollte oder könnte die Codeüberprüfung eine wertvolle Lernerfahrung für beide sein.

cseder
quelle
1

Die Codeüberprüfung bietet sowohl für Überprüfer als auch für Codierer die Möglichkeit, potenzielle Probleme zu erkennen und Wissen weiterzugeben.

Als Code-Reviewer liegt die Verantwortung darin, mögliche Risikobereiche, Nichteinhaltung der Standardpraxis, Verbesserungen und im Allgemeinen nur eine andere Sichtweise auf denselben Codebereich herauszustellen.

Dies sollte nicht zu einer Änderung des Codes führen, ohne die Entscheidungen des Codierers zum Zeitpunkt der Entwicklung zu erörtern / zu verstehen.

Wenn der Prüfer Änderungen vornimmt, kann es schwierig sein, die Arbeit an andere zu delegieren, was für viele kluge Köpfe schwierig ist.

Als Kodierer, der eine Überprüfung erhält, werden Sie bei der Bereitstellung vor einem möglichen Problem geschützt, haben die Möglichkeit, etwas Neues zu lernen und können Ihr Wissen mit dem Überprüfer teilen.

Unabhängig vom Dienstalter kann Ihre Denkweise zu einer Lösung führen, die für manche einfach nicht in Frage kommt. Die Überprüfung kann also Ihre Chance sein, zu glänzen, indem Sie einfach das tun, was Sie für richtig halten.

Solange sowohl der Kodierer als auch der Prüfer akzeptieren, dass sie falsch sein können und dass es in Ordnung ist, falsch zu liegen, stärkt eine Überprüfung das Team, da Sie gemeinsam an der Lösung arbeiten.

Kevin Hogg
quelle
0

Es sieht so aus, als hättest du deinen Code noch nicht überprüft :-)

Das Ziel der Codeüberprüfung ist es, Code in angemessener Qualität zu erhalten und zu wissen, dass Sie Code in angemessener Qualität haben. Wenn der Code eines unerfahrenen Entwicklers überprüft wird, kann er verwendet werden, um zu lehren, wie man besseren Code schreibt, ohne diesen Entwickler zu frustrieren.

Der Prüfer sollte Ihren Code niemals ändern. Sie können mehr oder weniger starke Vorschläge machen, wie Ihr Code geändert werden soll, und sie können entscheiden, ob sie Ihren Code akzeptieren oder nicht.

Wenn die Überprüfung richtig verläuft / wenn ich Ihren Code überprüfe, werden Sie wahrscheinlich einige Kommentare erhalten, wie ich den Code schreiben würde, aus dem Sie lernen oder ignorieren können andere Meinung. In meinem Bereich wird eine gute Benennung von Funktionen, Variablen usw. als wichtig angesehen, sodass Sie möglicherweise einige Vorschläge zur Verbesserung der Benennung erhalten. Normalerweise sollten Sie in diesem Fall Änderungen vornehmen (manchmal indem Sie einen noch besseren Namen für etwas finden). Manchmal finde ich Fehler. Sie reparieren sie. Manchmal finde ich Dinge, bei denen es sich um Fehler handelt, und ich liege falsch. Wenn Sie nur schwer erkennen können, dass der Code korrekt ist, machen Sie ihn offensichtlicher richtig. Wenn ich mich geirrt habe, sagst du es mir.

Wenn ich der Meinung bin, dass das Design im Allgemeinen nicht stimmt, hätte dies früher besprochen werden müssen. Wir sollten uns dann überlegen, ob Ihr Design gut genug ist, und dabei berücksichtigen, wie viel Arbeit mit einer Änderung verbunden ist, oder ob ich mich einfach geirrt habe und Ihr Design besser ist. Wir sollten uns einigen.

Wenn Rezensent und Rezensent nicht übereinstimmen können, haben wir ein Problem. Weil es bedeutet, dass einer von uns nicht in der Lage ist, im Team zu arbeiten, oder einer von uns nicht in der Lage ist, zwischen einem guten oder einem schlechten Design oder beidem zu unterscheiden. Dies ist nicht unbedingt deine Schuld. Leider gibt es Entwickler, die älter und ahnungslos sind, und das wird ein Problem für das Unternehmen und für Sie sein.

Wenn es passiert, überlegen Sie sehr, sehr genau: Haben Sie ein Problem damit, begründete Kritik anzunehmen? In diesem Fall müssen Sie Ihre Einstellung ändern. Sind Sie zu unerfahren, um zu sehen, warum der Rezensent Recht hat? Wenn das der Fall ist, ist das kein Problem. Vertraue dem Rezensenten und lerne. Sind Sie sicher, dass Sie es besser wissen als der Rezensent? Akzeptieren Sie die Überprüfung, aber fragen Sie einen dritten, vertrauenswürdigen Entwickler nach seiner Meinung. Denken Sie daran, Sie können sich wirklich sicher sein und Recht haben, aber Sie können sich auch wirklich sicher sein und sich irren.

gnasher729
quelle