Wie gehe ich mit Meinungsverschiedenheiten bei einer Codeüberprüfung in Bezug auf einen unwahrscheinlichen Randfall um?

Ich arbeite bei einem Robotik-Startup in einem Path-Coverage-Team und nach dem Absenden einer Pull-Anfrage wird mein Code überprüft.

Mein Teamkollege, der seit mehr als einem Jahr im Team ist, hat einige Kommentare zu meinem Code abgegeben, die darauf hindeuten, dass ich viel mehr arbeite, als ich für notwendig halte. Nein, ich bin kein fauler Entwickler. Ich liebe eleganten Code, der gute Kommentare, Variablennamen, Einrückungen und die richtige Behandlung von Fällen hat. Er hat jedoch eine andere Organisationsform im Sinn, der ich nicht zustimme.

Ich werde ein Beispiel geben:

Ich hatte einen Tag damit verbracht, Testfälle zu schreiben, um einen von mir vorgenommenen Algorithmus für die Übergangssuche zu ändern. Er hatte vorgeschlagen, dass ich mich mit einem obskuren Fall befasse, der äußerst unwahrscheinlich ist - tatsächlich bin ich mir nicht sicher, ob es überhaupt möglich ist, dass er auftritt. Der Code, den ich erstellt habe, funktioniert bereits in allen unseren Original-Testfällen und einigen neuen, die ich gefunden habe. Der Code, den ich erstellt habe, besteht bereits unsere über 300 Simulationen, die jede Nacht ausgeführt werden. Die Bearbeitung dieses undurchsichtigen Falls würde jedoch 13 Stunden in Anspruch nehmen, die ich besser verwenden könnte, um die Leistung des Roboters zu verbessern. Um klar zu sein, der bisher verwendete Algorithmus hat diesen obskuren Fall auch nicht behandelt und ist in den 40.000 Berichten, die generiert wurden, noch nie aufgetreten. Wir sind ein Startup und müssen das Produkt entwickeln.

Ich hatte noch nie zuvor eine Codeüberprüfung und bin mir nicht sicher, ob ich zu argumentativ bin. soll ich nur leise sein und tun, was er sagt? Ich beschloss, den Kopf gesenkt zu halten und die Änderung vorzunehmen, obwohl ich der Meinung bin, dass es eine gute Zeitnutzung war.

Ich respektiere meinen Kollegen und erkenne ihn als intelligenten Programmierer an. Ich bin in einem Punkt nicht mit ihm einverstanden und weiß nicht, wie ich mit Meinungsverschiedenheiten in einer Codeüberprüfung umgehen soll.

Ich bin der Meinung, dass die von mir gewählte Antwort diese Kriterien erfüllt, um zu erklären, wie ein Junior-Entwickler mit Meinungsverschiedenheiten in einer Codeüberprüfung umgehen kann.

Ein obskurer Fall, der äußerst unwahrscheinlich ist - tatsächlich bin ich mir nicht sicher, ob er überhaupt eintreten kann

Es kann sehr wichtig sein, nicht getestete Verhaltensweisen im Code zu haben. Wenn ein Teil des Codes z. B. 50 Mal pro Sekunde ausgeführt wird, erfolgt eine Eins-zu-eine-Million-Chance ungefähr alle 5,5 Stunden Laufzeit. (In Ihrem Fall scheinen die Chancen niedriger.)

Sie können mit Ihrem Vorgesetzten über die Prioritäten sprechen (oder mit einem Vorgesetzten, der für die Einheit, in der Sie arbeiten, verantwortlich ist). Sie werden besser verstehen, ob z. B. die Arbeit an der Codeleistung oder die Sicherung des Codes oberste Priorität hat und wie unwahrscheinlich dieser Eckfall sein kann. Ihr Prüfer hat möglicherweise auch eine falsche Vorstellung von den Prioritäten. Wenn Sie mit der verantwortlichen Person gesprochen haben, fällt es Ihnen leichter, sich mit den Vorschlägen Ihrer Prüfer (nicht) einverstanden zu erklären, und Sie müssen sich auf etwas beziehen.

Es ist immer eine gute Idee, mehr als einen Rezensenten zu haben. Wenn Ihr Code nur von einem Kollegen überprüft wird, bitten Sie jemanden, der den Code oder die Codebasis im Allgemeinen kennt, einen Blick darauf zu werfen. Eine zweite Meinung wird Ihnen wiederum helfen, den Vorschlägen des Rezensenten leichter (nicht) zuzustimmen.

Wenn während mehrerer Codeüberprüfungen mehrere Kommentare wiederholt werden, deutet dies in der Regel darauf hin, dass eine größere Sache nicht klar kommuniziert wird und immer wieder dieselben Probleme auftauchen. Versuchen Sie, diese größere Sache herauszufinden, und besprechen Sie sie direkt mit dem Rezensenten. Fragen Sie genug, warum Fragen. Es hat mir sehr geholfen, als ich angefangen habe, Code-Reviews durchzuführen.

Ich kann Ihnen ein Beispiel für einen Eckfall geben, der niemals eintreten könnte und eine Katastrophe verursachte.

Bei der Entwicklung der Ariane 4 wurden die Werte der Querbeschleunigungsmesser so skaliert, dass sie in eine 16-Bit-Ganzzahl mit Vorzeichen passen, und weil die maximal mögliche Ausgabe der Beschleunigungsmesser bei Skalierung 32767 niemals überschreiten und das Minimum niemals unterschreiten konnte. 32768 gab es "keine Notwendigkeit für den Overhead der Bereichsprüfung". Im Allgemeinen sollten alle Eingaben vor jeder Konvertierung auf ihren Bereich überprüft werden. In diesem Fall würde jedoch versucht, einen unmöglichen Eckfall zu ermitteln.

Einige Jahre später wurde die Ariane 5 entwickelt und der Code zur Skalierung der Querbeschleunigungsmesser wurde mit minimalem Testaufwand wiederverwendet, da er sich als "im Einsatz bewährt" erwies. Leider konnte die größere Rakete größere Querbeschleunigungen erwarten, so dass die Beschleunigungsmesser aufgerüstet wurden und größere 64-Bit- Float- Werte erzeugen konnten .

Diese größeren Werte „verpackt“ in dem Conversion - Code, denken Sie daran keine Bereichsprüfung, und die Ergebnisse auf dem ersten Start im Jahr 1996 waren nicht gut. Es kostete das Unternehmen Millionen und verursachte eine große Unterbrechung des Programms.

Der Punkt, den ich anstrebe, ist, dass Sie Testfälle nicht ignorieren sollten, da sie niemals vorkommen , äußerst unwahrscheinlich, etc .: Die Standards, für die sie codiert wurden, wurden für die Bereichsprüfung aller externen Eingabewerte aufgerufen . Wenn das getestet und gehandhabt worden wäre, wäre die Katastrophe möglicherweise abgewendet worden.

Beachten Sie, dass dies in Ariane 4 kein Fehler war (da für jeden möglichen Wert alles gut funktionierte) - es war ein Verstoß gegen die Standards. Wenn der Code in einem anderen Szenario wiederverwendet wurde, schlug er katastrophal fehl. Wenn der Wertebereich jedoch abgeschnitten worden wäre, wäre er wahrscheinlich ordnungsgemäß fehlgeschlagen, und das Vorhandensein eines Testfalls hierfür hätte möglicherweise eine Überprüfung der Werte ausgelöst. Es ist auch erwähnenswert, dass, während die Programmierer und Tester nach der Explosion Kritik von den Ermittlern einbrachten, das Management, die Qualitätssicherung und die Führung die Mehrheit der Schuld tragen mussten.

Klärung

Zwar ist nicht jede Software sicherheitskritisch und auch nicht so spektakulär, wenn sie versagt, aber ich wollte darauf hinweisen, dass "unmögliche" Tests immer noch einen Wert haben können. Dies ist der dramatischste Fall, den ich kenne, aber die Robotik kann auch katastrophale Folgen haben.

Persönlich würde ich sagen, dass, sobald jemand dem Testteam einen Eckfall vor Augen geführt hat, ein Test durchgeführt werden sollte, um dies zu überprüfen. Der Leiter des Implementierungsteams oder der Projektmanager kann beschließen, festgestellte Fehler nicht zu beheben, sollte sich jedoch dessen bewusst sein, dass Mängel bestehen. Wenn das Testen zu komplex oder zu teuer für die Implementierung ist, kann ein Problem in dem verwendeten Tracker und / oder dem Risikoregister auftreten, um zu verdeutlichen, dass es sich um einen nicht getesteten Fall handelt und möglicherweise behoben werden muss vor einer Nutzungsänderung oder Verhinderung einer unsachgemäßen Nutzung.

Da es vorher nicht behandelt wurde, liegt es außerhalb des Rahmens für Ihre Bemühungen. Sie oder Ihr Kollege können Ihren Vorgesetzten fragen, ob es sich lohnt, diesen Fall zu behandeln.

Mit komplexen Algorithmen ist es sehr schwierig zu beweisen, dass Sie an jeden Testfall gedacht haben, der in der realen Welt auftaucht. Wenn Sie einen Testfall absichtlich kaputt lassen, weil er in der realen Welt nicht auftritt, lassen Sie möglicherweise andere Testfälle kaputt, an die Sie noch nicht einmal gedacht haben.

Der andere Effekt, der häufig auftritt, besteht darin, dass Sie zusätzliche Testfälle behandeln, Ihr Algorithmus notwendigerweise allgemeiner wird und Sie daher Möglichkeiten finden, ihn zu vereinfachen und ihn für alle Ihre Testfälle robuster zu machen . Dies spart Ihnen Zeit bei der Wartung und Fehlerbehebung.

Außerdem gibt es unzählige Fälle, in denen "das sollte niemals passieren" -Fehler in freier Wildbahn auftreten. Dies liegt daran, dass sich Ihr Algorithmus möglicherweise nicht ändert, seine Eingaben sich jedoch möglicherweise in späteren Jahren ändern, wenn sich niemand mehr an diesen einen unbehandelten Anwendungsfall erinnert. Deshalb beschäftigen sich erfahrene Entwickler mit solchen Dingen, wenn sie frisch im Kopf sind. Es kommt zurück, um dich später zu beißen, wenn du es nicht tust.

Dies ist keine technische Frage, sondern eine Geschäftsstrategieentscheidung. Sie bemerken, dass die vorgeschlagene Lösung für einen sehr dunklen Fall gedacht ist, der so gut wie nie eintreten wird. Hier macht es einen großen Unterschied, ob Sie ein Spielzeug programmieren oder ob Sie beispielsweise medizinische Geräte oder eine bewaffnete Drohne programmieren. Die Folgen einer seltenen Fehlfunktion sind sehr unterschiedlich.

Wenn Sie Codeüberprüfungen durchführen, sollten Sie ein grundlegendes Verständnis der Geschäftsprioritäten anwenden, wenn Sie entscheiden, wie viel in die Behandlung seltener Fälle investiert werden soll. Wenn Sie mit Ihrem Kollegen bei der Interpretation der Prioritäten des Geschäfts nicht einverstanden sind, möchten Sie möglicherweise mit jemandem über die geschäftliche Seite der Dinge diskutieren.

Bei Code-Überprüfungen geht es nicht nur um Code-Korrektheit. In der Realität ist dies ziemlich weit unten auf der Liste der Vorteile, hinter dem Wissensaustausch und dem langsamen, aber stetigen Prozess zur Schaffung eines Konsenses zwischen Teamstil und Design.

Wie Sie erleben, ist oft umstritten, was als "richtig" gilt, und jeder hat seinen eigenen Blickwinkel darauf, was das ist. Nach meiner Erfahrung kann eine begrenzte Zeit und Aufmerksamkeit auch zu einer starken Inkonsistenz der Codeüberprüfungen führen - das gleiche Problem kann je nach Entwickler und zu unterschiedlichen Zeiten von demselben Entwickler aufgegriffen werden oder nicht. Rezensenten in der Denkweise von "Was würde diesen Code verbessern?" wird oft Änderungen vorschlagen, die sie auf natürliche Weise nicht zu ihren eigenen Anstrengungen hinzufügen würden.

Als erfahrener Codierer (mehr als 15 Jahre als Entwickler) werde ich oft von Codierern mit weniger Jahren Erfahrung als ich rezensiert. Manchmal fragen sie mich nach Änderungen, mit denen ich nicht einverstanden bin oder die ich für unwichtig halte. Ich nehme diese Änderungen jedoch immer noch vor. Ich kämpfe nur dann um Änderungen, wenn ich der Meinung bin, dass das Endergebnis zu einer Schwäche des Produkts führen würde, wenn der Zeitaufwand sehr hoch ist oder wenn ein "korrekter" Standpunkt objektiviert werden kann (z. B. wenn die Änderung erbeten wird). • Arbeiten Sie nicht in der Sprache, die wir verwenden, oder ein Benchmark zeigt, dass eine behauptete Leistungsverbesserung keine ist.

Also schlage ich vor, deine Schlachten sorgfältig zu wählen. Es lohnt sich wahrscheinlich nicht, einen Testfall, den Sie für nicht notwendig halten, zwei Tage lang zu codieren. Wenn Sie ein Überprüfungstool wie GitHub Pull Requests verwenden, kommentieren Sie dort möglicherweise den von Ihnen wahrgenommenen Kosten- / Nutzeneffekt, um Ihren Widerspruch zu vermerken, stimmen jedoch zu, die Arbeit dennoch auszuführen. Dies gilt als vorsichtiger Rückstoß, sodass der Prüfer weiß, dass er an eine Grenze stößt, und vor allem Ihre Überlegungen berücksichtigt, damit Fälle wie dieser eskaliert werden können, wenn sie in eine Sackgasse geraten. Sie möchten vermeiden, dass es zu eskalierenden schriftlichen Meinungsverschiedenheiten kommt - Sie möchten kein Argument im Stil eines Internetforums über Arbeitsunterschiede haben -, daher kann es hilfreich sein, das Problem zuerst durchzusprechen und eine angemessene Zusammenfassung des Ergebnisses der Diskussion aufzuzeichnen.freundschaftliches Gespräch wird für euch beide sowieso entscheiden).

Nach der Veranstaltung ist dies ein gutes Thema für Besprechungen zur Sprintüberprüfung oder zur Planung des Entwicklungsteams usw. Präsentieren Sie es so neutral wie möglich. Beispiel: "Bei der Codeüberprüfung hat Entwickler A diesen zusätzlichen Testfall identifiziert. Das Schreiben hat zusätzliche zwei Tage gedauert Ist das Team der Meinung, dass die zusätzliche Deckung zu diesem Preis gerechtfertigt war? " - Dieser Ansatz funktioniert viel besser, wenn Sie die Arbeit tatsächlich erledigen, da er Sie in einem positiven Licht zeigt. Sie haben die Arbeit erledigt und möchten das Team nur auf Risikoaversion und Funktionsentwicklung hin befragen.

Ich würde Ihnen raten, zumindest gegen den obskuren Fall vorzugehen. Auf diese Weise sehen zukünftige Entwickler nicht nur, dass Sie sich aktiv gegen den Fall entschieden haben, sondern bei einer guten Fehlerbehandlung, die bereits vorhanden sein sollte, würde dies auch für Überraschungen sorgen.

Machen Sie dann einen Testfall, der diesen Fehler bestätigt. Auf diese Weise wird das Verhalten besser dokumentiert und in Unit-Tests angezeigt.

Diese Antwort setzt natürlich voraus, dass Ihre Einschätzung, dass es "äußerst unwahrscheinlich, wenn überhaupt möglich" ist, richtig ist, und wir können das nicht beurteilen. Wenn dies jedoch der Fall ist und Ihr Kollege zustimmt, sollte eine ausdrückliche Behauptung gegen das Ereignis für Sie beide eine zufriedenstellende Lösung sein.

Da Sie dort neu zu sein scheinen, können Sie nur eines tun: Fragen Sie den Teamleiter (oder Projektleiter). 13 Stunden ist eine Geschäftsentscheidung; für einige Firmen / Teams viel; für manche nichts. Es ist noch nicht deine Entscheidung.

Wenn der Hinweis "Diesen Fall abdecken" lautet, ist das in Ordnung. Wenn er sagt "nee, scheiß drauf", gut - seine Entscheidung, seine Verantwortung.

Entspannen Sie sich bei Codeüberprüfungen im Allgemeinen. Es ist völlig normal, dass ein oder zwei Aufgaben an Sie zurückgesandt werden.

Eine Sache, von der ich glaube nicht, dass ich sie in Form von Sachleistungen gesehen habe, obwohl sie in der Antwort von @SteveBarnes aufgegriffen wurde:

Was sind die Auswirkungen eines Ausfalls?

In einigen Bereichen ist ein Fehler ein Fehler auf einer Webseite. Ein PC wird blau angezeigt und neu gestartet.

Auf anderen Gebieten ist es Leben oder Tod - selbstfahrendes Auto blockiert. Der medizinische Schrittmacher funktioniert nicht mehr. Oder in Steves Antwort: Sachen explodieren, was zum Verlust von Millionen von Dollar führt.

In diesen Extremen gibt es eine Welt voller Unterschiede.

Ob es sich letztendlich lohnt, 13 Stunden für einen "Misserfolg" aufzuwenden, sollte nicht an Ihnen liegen. Es sollte dem Management und den Eigentümern überlassen bleiben. Sie sollten ein Gespür für das Gesamtbild haben.

Sie sollten in der Lage sein, eine gute Vorstellung davon zu geben, was sich lohnen wird. Wird Ihr Roboter einfach langsamer oder anhalten? Beeinträchtigte Leistung? Oder wird ein Ausfall des Roboters einen finanziellen Schaden verursachen? Verlust des Lebens?

Die Antwort auf DIESE Frage sollte die Antwort auf "ist es 13 Stunden der Zeit des Unternehmens wert" treiben. Hinweis: Ich sagte der Firma Zeit. Sie bezahlen die Rechnungen und entscheiden letztendlich, was es wert ist. Ihr Management sollte das letzte Wort haben.

Vielleicht mit der Person sprechen, die für die Priorisierung der Arbeit verantwortlich ist? Im Startup könnte CTO oder Product Owner sein. Er könnte helfen, herauszufinden, ob und warum diese zusätzliche Arbeit erforderlich ist. Sie könnten auch Ihre Sorgen während der täglichen Stand-ups mitbringen (wenn Sie sie haben).

Wenn keine klare Verantwortung (z. B. Produktbesitzer) für die Planung von Arbeiten besteht, versuchen Sie, mit Personen in Ihrer Umgebung zu sprechen. Später könnte es zu einem Problem werden, dass jeder das Produkt in die entgegengesetzte Richtung treibt.

Der beste Weg, mit Meinungsverschiedenheiten umzugehen, ist der gleiche, unabhängig davon, ob Sie ein Junior-Entwickler, ein Senior-Entwickler oder sogar ein CEO sind.

Benimm dich wie Columbo .

Wenn Sie noch nie ein Columbo gesehen haben, war es eine ziemlich fantastische Show. Columbo war ein sehr bescheidener Charakter - die meisten Leute dachten, er sei ein bisschen verrückt und es nicht wert, sich Sorgen zu machen. Aber indem er demütig wirkte und die Leute nur aufforderte zu erklären, konnte er seinen Mann holen.

Ich denke, es hängt auch mit der sokratischen Methode zusammen .

Im Allgemeinen möchten Sie sich und anderen Fragen stellen, um sicherzustellen, dass Sie die richtigen Entscheidungen treffen. Nicht aus der Position "Ich habe Recht, du liegst falsch", sondern aus der Position der ehrlichen Entdeckung. Oder zumindest so gut wie du kannst.

In Ihrem Fall haben Sie hier zwei Ideen, die jedoch im Wesentlichen dasselbe Ziel verfolgen: den Code zu verbessern.

Sie haben den Eindruck, dass die Reduzierung der Codeabdeckung für einen möglicherweise unwahrscheinlichen (unmöglichen?) Fall zugunsten der Entwicklung anderer Funktionen der beste Weg ist, dies zu tun.

Ihr Mitarbeiter hat den Eindruck, dass es wertvoller ist, mit den Eckkoffern vorsichtiger umzugehen.

Was sehen sie, was du nicht siehst? Was siehst du, was sie nicht sehen? Als Junior-Entwickler sind Sie in einer großartigen Position, weil Sie natürlich Fragen stellen sollten. In einer anderen Antwort erwähnt jemand, wie überraschend wahrscheinlich ein Eckfall ist. Sie können also mit "Hilf mir zu verstehen - ich hatte den Eindruck, dass X, Y und Z - was fehlt? Warum wird das Widget durcheinander geraten? Ich hatte den Eindruck, dass es unter kritischen Umständen funktioniert. Will der Swizzle Stick tatsächlich die ANZA-Bürsten anreichert? "

Wenn Sie Ihre Vermutungen und Ergebnisse in Frage stellen, werden Sie nach unten graben, Vorurteile aufdecken und schließlich herausfinden, wie Sie richtig vorgehen.

Beginnen Sie mit der Annahme, dass jeder in Ihrem Team vollkommen vernünftig ist und die besten Interessen des Teams und des Produkts im Auge hat, genau wie Sie. Wenn sie etwas tun, das keinen Sinn ergibt, müssen Sie herausfinden, was sie nicht wissen oder was sie nicht wissen.

13 Stunden sind keine so große Sache, ich würde es einfach tun. Denken Sie daran, dass Sie dafür bezahlt werden. Kreide es einfach als "Arbeitsplatzsicherheit" an. Auch ist es am besten, gutes Karma im Team zu behalten. Wenn es etwas wäre, das Sie eine Woche oder länger in Anspruch nehmen würde, könnten Sie Ihren Vorgesetzten einbeziehen und ihn fragen, ob dies die beste Verwendung Ihrer Zeit ist, insbesondere wenn Sie nicht damit einverstanden sind.

Sie scheinen jedoch eine Hebelwirkung in Ihrer Gruppe zu benötigen. So erhalten Sie eine Hebelwirkung: Bitten Sie um Vergebung, bitten Sie nicht um Erlaubnis. Ergänzen Sie das Programm nach Belieben (im Rahmen des Kurses, dh stellen Sie sicher, dass es das vom Chef gewünschte Problem vollständig löst), und teilen Sie dies dem Manager oder Ihren Kollegen im Nachhinein mit. Fragen Sie sie nicht: "Ist es in Ordnung, wenn ich Feature X hinzufüge?" Fügen Sie stattdessen einfach Funktionen hinzu, die Sie persönlich im Programm haben möchten. Wenn sie sich über eine neue Funktion aufregen oder nicht damit einverstanden sind, können Sie sie entfernen. Wenn sie es mögen, behalten Sie es.

Darüber hinaus gehen Sie immer dann, wenn Sie aufgefordert werden, etwas zu tun, die "Extrameile" und fügen Sie eine Menge Dinge hinzu, die Sie vergessen haben, zu erwähnen, oder Dinge, die besser funktionieren als die, die Sie gesagt haben. Aber frag sie nicht, ob es in Ordnung ist, die Extrameile zu gehen. Tu es einfach und erzähle ihnen beiläufig davon, nachdem es fertig ist. Was Sie tun, trainiert sie ..

Was passiert, ist, dass Ihr Manager Sie als "Macher" ansieht und Ihnen sein Vertrauen schenkt, und Ihre Kollegen werden Sie als die Führungskraft sehen, die Sie zu besitzen beginnen. Und dann, wenn Dinge wie das, was Sie erwähnen, in Zukunft passieren, werden Sie mehr Mitspracherecht haben, weil Sie im Wesentlichen der Star des Teams sind und Teamkollegen nachgeben, wenn Sie ihnen nicht zustimmen.

Die Codeüberprüfung dient mehreren Zwecken. Eine, die Ihnen offensichtlich bewusst ist, ist: " Ist dieser Code zweckmäßig? " Mit anderen Worten, ist er funktionell korrekt; ist es ausreichend getestet; sind die nicht offensichtlichen Teile angemessen kommentiert; Entspricht es den Konventionen des Projekts?

Ein weiterer Teil der Codeüberprüfung ist das Teilen von Wissen über das System. Es ist eine Gelegenheit für den Autor und den Rezensenten, mehr über den geänderten Code und dessen Interaktion mit dem Rest des Systems zu erfahren.

Ein dritter Aspekt besteht darin, dass eine Überprüfung der Probleme möglich ist, die vor dem Vornehmen von Änderungen aufgetreten sind. Sehr oft stelle ich bei der Überprüfung der Änderungen eines anderen fest, dass ich in einer vorherigen Iteration etwas übersehen habe (ziemlich oft etwas Eigenes). Eine Aussage wie "Hier ist eine Gelegenheit, dies robuster zu machen, als es war" ist keine Kritik, und nehmen Sie es nicht als eine!

Mein derzeitiges Team betrachtet die Codeüberprüfung nicht nur als ein Gateway oder eine Hürde, die der Code vor dem Festschreiben unbeschadet bestehen muss, sondern in erster Linie als Gelegenheit für eine etwas strukturierte Diskussion über einen bestimmten Funktionsbereich. Dies ist eine der produktivsten Gelegenheiten für den Informationsaustausch. (Und das ist ein guter Grund, die Bewertung im Team zu teilen und nicht immer derselbe Prüfer).

Wenn Sie Code Reviews als konfrontative Aktivität wahrnehmen, ist dies eine sich selbst erfüllende Prophezeiung. Wenn Sie sie stattdessen als den kollaborativsten Teil Ihrer Arbeit betrachten, werden sie zu kontinuierlichen Verbesserungen Ihres Produkts und Ihrer Zusammenarbeit anregen. Es ist hilfreich, wenn in einer Überprüfung die relativen Prioritäten der Vorschläge klar definiert werden können - es gibt beispielsweise einen Unterschied zwischen "Ich möchte hier einen hilfreichen Kommentar" und "Dies bricht ab, wenn xes jemals negativ ist".

Wie trifft dies auf Ihre spezifische Situation zu? Ich hoffe, es ist jetzt offensichtlich, dass mein Rat darin besteht, auf die Überprüfung mit offenen Fragen zu antworten und zu verhandeln, welcher Ansatz den größten Wert hat. In Ihrem Beispielfall, in dem ein zusätzlicher Test vorgeschlagen wird, lautet der Test wie folgt: "Ja, das können wir testen. Ich schätze , die Implementierung wird <Zeit> in Anspruch nehmen. Glauben Sie, dass sich der Nutzen lohnt? Und gibt es noch etwas, das wir tun?" kann man dafür sorgen, dass der Test nicht nötig ist? "

Eine Sache fällt mir auf, wenn ich Ihre Frage lese: Wenn es zwei Tage dauert, einen neuen Testfall zu schreiben, dann ist entweder Ihr neuer Test ein ganz anderes Szenario als Ihre vorhandenen Tests (in diesem Fall hat er wahrscheinlich eine Menge Wert) oder Sie haben festgestellt, dass die Wiederverwendung von Code in der Testsuite verbessert werden muss.

Als allgemeinen Kommentar zum Wert von Code Reviews (und als wichtige Zusammenfassung dessen, was ich oben gesagt habe) gefällt mir diese Aussage in Maintainers Don't Scale von Daniel Vetter :

Zumindest für mich geht es bei der Überprüfung nicht nur darum, eine gute Codequalität sicherzustellen, sondern auch darum, Wissen zu verbreiten und das Verständnis zu verbessern. Zuerst gibt es vielleicht eine Person, den Autor (und das ist keine Selbstverständlichkeit), der den Code versteht. Nach einer guten Überprüfung sollte es mindestens zwei Personen geben, die es vollständig verstehen, einschließlich der Eckfälle.

Code kann IMMER besser sein.

Wenn Sie sich in einer Codeüberprüfung befinden und nichts Besseres oder keinen Komponententest sehen, der einen Fehler entdeckt, ist nicht der Code perfekt, sondern der Prüfer, der seine Arbeit nicht erledigt. Ob Sie die Verbesserung erwähnen, ist eine persönliche Entscheidung. Aber fast jedes Mal, wenn Ihr Team eine Codeüberprüfung durchführt, sollte es Dinge geben, die jemand bemerkt, die besser sein könnten, oder jeder sollte wahrscheinlich nur seine Zeit verschwenden.

Das heißt, ob Sie auf die Kommentare reagieren oder nicht, liegt bei Ihrem Team. Wenn Ihre Änderungen das Problem beheben oder den Wert erhöhen, ohne dass Ihr Team sie akzeptiert, führen Sie sie zusammen und protokollieren Sie ihre Kommentare im Backlog, damit sie später von jemandem bearbeitet werden können. Wenn das Team feststellt, dass Ihre Änderungen mehr Risiko oder Komplexität als Wert bedeuten, müssen Sie die Probleme entsprechend lösen.

Denken Sie daran, dass jeder Code mindestens einen Edge-Case mehr hat, der getestet werden könnte und mindestens ein Refactoring mehr benötigt. Dies ist der Grund, warum Codeüberprüfungen am besten in einer Gruppe durchgeführt werden, in der jeder zur gleichen Zeit denselben Code betrachtet. Damit sich am Ende alle einig werden können, ob der überprüfte Code akzeptabel ist (wie er ist) und genug Wert zum Zusammenführen in die Community-Basis hinzufügt oder ob bestimmte Dinge getan werden müssen, bevor es genug Wert zum Zusammenführen gibt .

Da Sie diese Frage stellen, gehe ich davon aus, dass Sie eigentlich keine "Codeüberprüfungen" durchführen, sondern stattdessen eine Pull-Anfrage oder einen anderen Übermittlungsmechanismus erstellen, damit andere Personen nicht deterministisch kommentieren. Nun haben Sie ein Managementproblem und eine Definition von „erledigt“. Ich nehme an, Ihr Management ist unentschlossen und versteht den Prozess und den Zweck der Codeüberprüfung nicht und hat wahrscheinlich keine "Definition von erledigt" (DOD). Denn wenn sie es taten, würde Ihr DOD diese Frage explizit beantworten und Sie müssten nicht hierher kommen und fragen.

Wie beheben Sie das? Bitten Sie Ihren Manager, Ihnen ein DOD zu geben und Ihnen mitzuteilen, ob Sie immer alle Kommentare implementieren müssen. Wenn die betreffende Person Ihr Vorgesetzter ist, ist die Antwort selbstverständlich.

Bei dieser Frage geht es nicht um die Vorteile der defensiven Programmierung, die Gefahren von Eckfällen oder die katastrophalen Risiken von Fehlern in physischen Produkten. Tatsächlich geht es überhaupt nicht wirklich um Software-Engineering .

Worum es wirklich geht, ist, wie ein Junior-Praktizierender eine Anweisung von einem Senior-Praktizierenden bearbeitet, wenn der Junior nicht zustimmen oder es schätzen kann.

Es gibt zwei Dinge, die Sie als Junior-Entwickler zu schätzen wissen müssen. Erstens bedeutet dies, dass es zwar möglich ist, dass Sie im Recht und er im Unrecht sind, es jedoch - in Abwägung der Wahrscheinlichkeiten - unwahrscheinlich ist. Wenn Ihr Kollege einen Vorschlag macht, dessen Wert Sie nicht erkennen können, müssen Sie ernsthaft die Möglichkeit in Betracht ziehen, dass Sie nicht genug Erfahrung haben, um ihn zu verstehen. Ich verstehe das nicht aus diesem Beitrag.

Das zweite, was Sie zu schätzen wissen, ist, dass Ihr Senior-Partner so genannt wird, weil er mehr Verantwortung trägt. Wenn ein Junior etwas Wichtiges kaputt macht, bekommt er keine Probleme, wenn er die Anweisungen befolgt. Wenn ein Senior es ihnen jedoch erlaubt , es zu brechen, zum Beispiel indem er keine Probleme bei der Codeüberprüfung aufwirft, würden sie zu Recht viel Mühe haben.

Letztendlich ist es eine implizite Anforderung Ihrer Arbeit, dass Sie die Anweisungen derjenigen befolgen, denen das Unternehmen vertraut, um ihre Projekte zu leiten. Können Sie sich im Allgemeinen nicht an Senioren halten, wenn es einen guten Grund gibt, ihre Erfahrung zu schätzen? Beabsichtigen Sie, keine Anweisungen zu befolgen, die Sie nicht verstehen können? Denken Sie, die Welt sollte aufhören, bis Sie überzeugt sind? Diese Werte sind nicht mit der Arbeit in einem Team vereinbar.

Ein letzter Punkt. Denken Sie an die Projekte zurück, die Sie vor sechs Monaten geschrieben haben. Denken Sie an die Projekte, die Sie an der Universität geschrieben haben. Sehen Sie, wie schlimm sie jetzt aussehen - all die Bugs und das auf den Kopf gestellte Design, die blinden Flecken und die fehlgeleiteten Abstraktionen? Was wäre, wenn ich Ihnen sagen würde, dass Sie in sechs Monaten genau die Fehler in der Arbeit zählen, die Sie heute machen? Zeigt das, wie viele blinde Flecken in Ihrer aktuellen Herangehensweise vorkommen? Denn das ist der Unterschied, den Erfahrung macht.

macht ständig Kommentare zu meinem Code, die darauf hindeuten, dass ich viel mehr Arbeit mache als nötig.

Sie können Empfehlungen geben, aber letztendlich ist es nicht Ihre Aufgabe, zu entscheiden, was notwendig ist. Es ist Ihre Aufgabe, umzusetzen, was das Management oder (in diesem Fall Ihr Prüfer) entscheidet, ist notwendig. Wenn Sie mit dem, was zu viel oder zu stark erforderlich ist, nicht einverstanden sind, werden Sie wahrscheinlich arbeitslos. Folglich ist es Teil Ihrer Professionalität, sich damit abzufinden und damit im Frieden zu sein.

Er hatte vorgeschlagen, dass ich mich mit einem obskuren Fall befasse, der äußerst unwahrscheinlich ist

Es gibt noch andere großartige Antworten, die zeigen, wie manchmal sogar Fehler, die nicht auf Bugs zurückzuführen sind (dh etwas, das nachweislich niemals versagen kann ), noch nachgearbeitet werden sollten. (z. B. im Fall der zukünftigen Sicherheit des Produkts, der Einhaltung von Standards usw.) Es gehört zur Rolle eines großen Entwicklers, so sicher wie möglich zu sein, dass Ihr Code jederzeit und in Zukunft in jeder denkbaren Situation robust ist Außerdem ist es nicht nur sicher, sondern arbeitet die meiste Zeit unter den gegenwärtigen Bedingungen in erprobten Situationen

Vorschläge an die Codeüberprüfer, um den geschäftlichen Nutzen Ihrer Codeüberprüfung zu erhöhen (Sie als OP sollten eine solche Änderung vorschlagen):

• Notieren Sie Ihre Kommentare nach Typ. "Kritisch" / "Muss" / "Optional" / "Verbesserungsvorschläge" / "Schön zu haben" / "Ich denke nach".

  Wenn dies zu CDO / anal / kompliziert erscheint, verwenden Sie mindestens zwei Ebenen: "Muss korrigiert werden, um die Überprüfung zu bestehen, und darf Ihre Änderung zusammenführen" / "Alle anderen".

Vorschläge für den Umgang mit Vorschlägen zur Codeüberprüfung, die weniger kritisch erscheinen:

• Erstellen Sie ein offenes Ticket in einem Ticketing-System Ihrer Wahl (Ihr Team verwendet hoffentlich eines?) Und verfolgen Sie den Vorschlag

• Fügen Sie die Ticketnummer als Antwortkommentar zum Code-Überprüfungselement hinzu, wenn Ihr Prozess Antworten auf Kommentare wie Fisheye oder E-Mail-Überprüfungen zulässt.

• Wenden Sie sich an den Prüfer und fragen Sie explizit, ob dieser Artikel vom Typ "Muss behoben werden oder wird nicht zusammengeführt / freigegeben" ist.

  • Wenn die Antwort "Ja" lautet, Sie jedoch nicht einverstanden sind, lassen Sie die für das Projektmanagement verantwortliche Person (PM, Ihren Manager usw.) eine Entscheidung treffen - und legen Sie die Meinungsverschiedenheit vollständig und ehrlich dar. Es geht nicht darum, wer von Ihnen "richtig" ist, sondern darum, was für das Projekt besser ist, also der Job des PM / Managers.

Behandeln Sie dieses Ticket nun wie jede andere Entwicklungsanforderung.

1. Wenn es nach einer Eskalation dringend sein soll, behandeln Sie es als dringende Dev-Anfrage. Entpriorisieren Sie andere Arbeiten und arbeiten Sie daran.

2. Andernfalls bearbeiten Sie es entsprechend der ihm zugewiesenen Priorität und seiner Rendite (die je nach Geschäftsbereich unterschiedlich sein kann, wie in anderen Antworten erläutert).

Sie sollten dies nicht an das Management weiterleiten.

In den meisten Unternehmen hat sich der Manager immer dafür entschieden, diesen zusätzlichen Test nicht zu schreiben, keine Zeit damit zu verbringen, die Codequalität geringfügig zu verbessern und keine Zeit für das Refactoring zu verlieren .

In vielen Fällen hängt die Codequalität von den ungeschriebenen Regeln im Entwicklungsteam und dem zusätzlichen Aufwand ab, den die Programmierer leisten.

Sie sind ein Junior-Entwickler und dies ist Ihre erste Codeüberprüfung . Sie müssen den Rat annehmen und die Arbeit erledigen. Sie können den Workflow und die Regeln in Ihrem Team nur verbessern, wenn Sie sie zuerst kennen und eine Weile respektieren, damit Sie verstehen, warum sie dort sind. Sonst bist du der Neue, der sich nicht an die Regeln hält und der einzige Wolf des Teams wird.

Sie sind neu im Team, folgen den Ratschlägen, die Sie für eine Weile erhalten, finden heraus, warum sie da sind, und bringen Sie nicht die ersten Ratschläge mit, die Sie beim Scrum-Meeting in Frage stellen. Die wirklichen Geschäftsprioritäten werden Ihnen nach einer Weile ohne Rückfrage klar (und möglicherweise nicht das, was Ihnen der Manager von Angesicht zu Angesicht mitteilt).

Es ist sehr wichtig, dass Sie Code erstellen, der Ihre Lead- / Management-Anforderung erfüllt. Jedes andere Detail wäre nur ein "nice to have". Wenn Sie ein Experte auf Ihrem Gebiet sind (lesen Sie: "Wenn Sie kein Junior-Entwickler sind"), sind Sie "berechtigt", kleinere Probleme, die Sie unterwegs finden, zu lösen, ohne jedes Mal Ihre Führungskräfte zu konsultieren.

Wenn Sie der Meinung sind, dass etwas nicht in Ordnung ist und Sie auf Ihrem Gebiet relativ erfahren sind, stehen die Chancen gut, dass Sie Recht haben .

Hier sind einige Aussagen, die für Sie nützlich sein könnten:

• Ich wurde gebeten, X zu machen. Der Code-Prüfer schlägt vor, auch Y zu machen. Sollte ich es tun oder sollte ich zu wichtigeren Dingen übergehen?
• Sie schlagen mir Y vor. Können Sie also mindestens einen Testfall finden, der dieses Verhalten auffängt, damit ich es testen kann? Ich glaube, dass der Code nicht aufgerufen wird.
• Vielleicht sollten wir nicht zuerst einen sicheren Fallback für ungedeckte Fälle entwickeln? Also fangen wir sie frühzeitig und reparieren sie unterwegs, damit wir uns auf wichtigere Dinge konzentrieren können.
• OK, während ich Y implementiere, könnten Sie so freundlich sein, einige Testfälle dafür zu schreiben, damit wir das Ding ein für alle Mal erledigen können ?
• Ich wurde gebeten, X zu machen, und ich denke, ich könnte Y machen, wenn es keine anderen Prioritäten gibt. Warum reichen Sie beim nächsten Mal keine Funktionsanforderung ein, anstatt sie als Überprüfungskommentar in meinen Code einzufügen ? Zumindest können wir weitere Meinungen von anderen Teammitgliedern zu diesem Feature hören, bevor wir es implementieren (im Allgemeinen sollten alle wichtigen Dinge ein Feature sein und von mehr als einer Person behandelt werden; in der Regel sollte es bei der Codeüberprüfung darum gehen, Code- und Lösungsansätze zu überprüfen; der Rest ist Bugfixing und Features).

Denken Sie ernsthaft, dass die Einstellung Ihres Reviewers das Projekt schädigt, oder denken Sie, dass er die meiste Zeit Recht hat (außer vielleicht macht er manchmal nur geringfügige Fehler bei der Bewertung und übertreibt dies)?

Für mich klingt es eher so, als würde er Dinge schreiben, die nicht zu einer Codeüberprüfung gehören, was eine schlechte Praxis ist, da es für alle schwieriger ist, Dinge zu verfolgen. Wie auch immer, ich weiß nicht, welche anderen Kommentare er gemacht hat, daher kann ich zu anderen Fällen nichts sagen.

Versuchen Sie im Allgemeinen, beide der folgenden Punkte zu vermeiden:

• Sie tun nicht das, was angefordert wurde
• Sie geben Ihrem Rezensenten das Gefühl, dumm zu sein

Wenn er Ihren Code tatsächlich überprüft, liegt es daran, dass das Management ihm mehr vertraut als Ihnen.

Wenn es während der Codeüberprüfung zu Meinungsverschiedenheiten über den Umfang kommt:

1. Dokumentieren Sie den Geltungsbereich, der tatsächlich vom Code abgedeckt wird. Niemand mag böse Überraschungen.
2. Erkennen Sie, dass der Umfang eine Geschäftsentscheidung ist. Der Umfang sollte bereits bekannt sein, wenn Sie mit der Arbeit an einem Feature beginnen. Wenn dies nicht der Fall ist, können Sie zu einem späteren Zeitpunkt weitere Informationen anfordern.

Wenn der Codeüberprüfer die Geschäftsentscheidungen trifft, kann er den Umfang jederzeit ändern, auch während der Codeüberprüfung, dies tut er jedoch in seiner Rolle als Codeüberprüfer nicht.

Wenn Sie nicht beweisen können, dass der Edge Case unmöglich ist, müssen Sie davon ausgehen, dass er möglich ist. Wenn es möglich ist, ist es unvermeidlich, dass es irgendwann und eher früher als später auftritt. Wenn der Randfall beim Testen nicht aufgetreten ist, kann dies ein Hinweis darauf sein, dass die Testabdeckung unvollständig ist.

1. Akzeptiere das Feedback.
2. Bevor Sie Änderungen an Ihrem Code vornehmen, versuchen Sie, einen Test für den Edge-Fall zu erstellen und festzustellen, ob ein Testfehler auftreten kann (Beweis dafür, dass das Problem vorliegt). Wenn es unmöglich ist, einen solchen Testfall zu erstellen und einen Testfehler zu erhalten, können Sie möglicherweise die Schlussfolgerung ziehen, dass der Edge-Fall tatsächlich unmöglich ist (obwohl ich zögern würde, eine solche Schlussfolgerung zu ziehen).
3. Wenn Sie einen Testfehler erhalten, wenden Sie die entsprechende Codeänderung an, um den Test zu bestehen.

Zum Wohle des Produkts möchten Sie wahrscheinlich in der Lage sein, das Edge-Case zu produzieren und einen Fehler auszulösen, damit Sie den Fix anwenden und darauf vertrauen können, dass ein potenzielles Problem abgewendet wurde.

Der springende Punkt bei Code-Überprüfungen ist, dem Code zusätzliche Aufmerksamkeit zu widmen. Keiner von uns ist immun gegen Fehler oder Versehen. Es ist allzu üblich, sich einen Code mehrmals anzusehen und keinen offensichtlichen Fehler zu bemerken, bei dem ein neues Paar Augen ihn sofort erkennen kann.

Wie Sie sagten, haben Sie einen neuen Algorithmus implementiert. Es wäre unklug, Schlussfolgerungen über das Verhalten Ihres neuen Algorithmus zu ziehen, die auf dem Verhalten oder den Beobachtungen zu seinem Vorgänger beruhen.

Es gibt Code-Prüfer, die wissen, was sie tun, und wenn sie sagen, dass etwas geändert werden muss, muss es geändert werden, und wenn sie sagen, dass etwas getestet werden muss, muss es getestet werden.

Es gibt Code-Reviewer, die ihre eigene Existenz rechtfertigen müssen, indem sie nutzlose Arbeit für andere schaffen.

Welches ist welches für Sie, und wie mit der zweiten Art umzugehen ist, ist eher eine Frage für Workplace.stackexchange.

Wenn Sie Scrum verwenden, ist die Frage, ob Ihre Arbeit das tut, was sie tun soll (anscheinend auch), und Sie können den äußerst seltenen und möglicherweise unmöglichen Fall in den Rückstand aufnehmen, wo er priorisiert wird, und ob er priorisiert wird Geht ein Sprint los, dann kann Ihr Reviewer ihn abholen und die 13-Stunden-Arbeit erledigen. Wenn Sie Job X ausführen und weil Sie Job X ausführen, stellen Sie fest, dass Job Y ebenfalls ausgeführt werden muss, dann wird Job Y nicht Teil von Job X, sondern ist ein eigenständiger Job.