Sollte der Prüfer bei Codeüberprüfungen immer eine Lösung für Probleme vorlegen? [geschlossen]

93

Beim Überprüfen von Code versuche ich normalerweise, bestimmte Empfehlungen zur Behebung der Probleme abzugeben. Aufgrund der begrenzten Zeit, die für die Überprüfung aufgewendet werden kann, funktioniert dies jedoch nicht immer gut. In diesen Fällen finde ich es effizienter, wenn der Entwickler selbst eine Lösung findet.

Heute habe ich einen Code überprüft und festgestellt, dass eine Klasse offensichtlich nicht gut gestaltet war. Es hatte eine Reihe von optionalen Attributen, die nur für bestimmte Objekte zugewiesen und für andere leer gelassen wurden. Die Standardmethode zur Behebung dieses Problems besteht darin, die Klasse aufzuteilen und die Vererbung zu verwenden. In diesem speziellen Fall schien diese Lösung die Dinge jedoch zu komplizieren. Ich war selbst nicht an der Entwicklung dieser Software beteiligt und bin nicht mit allen Modulen vertraut. Daher fühlte ich mich nicht sachkundig genug, um eine bestimmte Entscheidung zu treffen.

Ein anderer typischer Fall, den ich oft erlebt habe, ist, dass ich eine offensichtlich bedeutungslose oder sogar irreführende Funktion, einen Klassen- oder Variablennamen finde, aber selbst keinen guten Namen finden kann.

Ist es als Rezensent im Allgemeinen in Ordnung zu sagen, dass "dieser Code fehlerhaft ist, weil ..., machen Sie es anders" oder müssen Sie sich eine bestimmte Lösung einfallen lassen?

Frank Puffer
quelle
24
@gnat: Nein, der Code ist nicht zu kompliziert. Und es ist nur ein Beispiel. Ich frage allgemein, ob der Prüfer für die Vorlage einer Lösung verantwortlich ist.
Frank Puffer
5
nein, ich würde sagen, dass Sie als Rezensent nicht gezwungen sind, Verbesserungsvorschläge zu machen. Wenn Sie beschreiben können, was sich dort falsch anfühlt, tun Sie es; Wenn nicht, geben Sie einfach einen allgemeinen Kommentar ab. (Einer der nützlichsten Bewertungskommentare, an die ich mich erinnere, war buchstäblich wie "diese Klasse ist alles Müll")
Mücke
5
Msgstr "Die Standardmethode zur Lösung dieses Problems wäre, die Klasse aufzuteilen und die Vererbung zu verwenden." Ich hoffe, Sie überprüfen meinen Code nicht!
Gardenhead
7
Das Auffinden möglicher Probleme könnte ausreichen. Der Prüfer betrachtet den Code als Benutzer, Betreuer oder Designer. Wenn der Codierer einen anderen Blickwinkel oder andere Probleme beim Erkennen hat, kann er seine Arbeit möglicherweise noch verbessern. Und wenn Sie etwas entdecken, das Ihnen gefällt, würde es nicht schaden, das auch zu melden. Es sollte keine Korrekturübung sein, sondern eine aufschlussreiche. Aus diesem Grund wird es "Peer Review" genannt.
Martin Maat

Antworten:

139

Als Prüfer müssen Sie überprüfen, ob ein Teil des Codes (oder ein Dokument) bestimmte Ziele erfüllt, die vor der Prüfung vereinbart wurden.

Einige dieser Ziele beinhalten normalerweise eine Beurteilung, ob das Ziel erreicht wurde oder nicht. Zum Beispiel erfordert das Ziel, dass der Code gewartet werden muss, normalerweise einen Entscheidungsaufruf.

Als Rezensent ist es Ihre Aufgabe, darauf hinzuweisen, wo die Ziele nicht erreicht wurden, und es ist die Aufgabe des Autors, sicherzustellen, dass seine Arbeit tatsächlich den Zielen entspricht. Auf diese Weise ist es nicht Ihre Aufgabe zu sagen, wie die Korrekturen vorgenommen werden müssen.

Auf der anderen Seite führt es in der Regel nicht zu einer positiven Atmosphäre im Team, wenn dem Autor nur gesagt wird, dass dies fehlerhaft ist. Für eine positive Atmosphäre ist es gut, zumindest anzugeben, warum etwas in Ihren Augen fehlerhaft ist, und eine bessere Alternative bereitzustellen, wenn Sie eine haben.
Abgesehen davon, wenn Sie etwas überprüfen, das "falsch" aussieht, aber keine wirklich bessere Alternative hat, dann könnten Sie auch einen Kommentar in der Art von "Dieser Code / Entwurf passt nicht gut zu mir, aber ich" hinterlassen Wir haben keine klare Alternative. Können wir das diskutieren? " und dann versuchen, etwas besser zusammen zu bekommen.

Bart van Ingen Schenau
quelle
23
+1 für die Diskussion, um gemeinsam eine Lösung zu finden - so lerne ich am meisten von den erfahrenen Programmierern, die meinen Code überprüfen
dj18
19
+1. Wenn Sie Feedback geben, ist es am besten, wenn möglich konstruktive Kritik zu üben.
FrustratedWithFormsDesigner
7
Dem letzten Teil stimme ich besonders zu. Es ist völlig in Ordnung zu sagen, "diese Lösung fühlt sich falsch an, weil ..." oder "ich mache mir Sorgen, dass dieser Teil problematisch sein könnte, weil ...", ohne eine Lösung anzugeben.
Daniel T.
1
@dotancohen: Das "Können wir das diskutieren" sollte eine Frage sein. Persönlich hätte ich die Diskussion trotzdem, auch wenn es nur darum geht, selbst etwas zu lernen.
Bart van Ingen Schenau
1
Die subtile Gefahr besteht darin, dass dies bei ausreichender Diskussion und Kommunikation mit der Implementierung nicht mehr zu einer Überprüfung, sondern zu einer Paarprogrammierung wird. Die Paarprogrammierung ist gut, aber sobald dies erledigt ist, muss eine dritte Person überprüft werden, da die Unabhängigkeit verloren gegangen ist.
candied_orange
35

Einige gute Antworten hier, aber ich denke, ein wichtiger Punkt fehlt. Es macht einen großen Unterschied, welchen Code Sie überprüfen, wie erfahren diese Person ist und wie sie mit solchen Vorschlägen umgeht. Wenn Sie Ihren Teamkollegen gut kennen und eine Notiz wie "Dieser Code ist fehlerhaft, weil ..., anders machen" erwarten , die ausreicht, um eine bessere Lösung zu finden, kann ein solcher Kommentar in Ordnung sein. Aber es gibt definitiv Personen, bei denen ein solcher Kommentar nicht ausreicht und denen genau gesagt werden muss, wie sie ihren Code verbessern können. IMHO ist dies ein Urteilsruf, den Sie nur für den Einzelfall machen können.

Doc Brown
quelle
29

Ist es für einen Rezensenten im Allgemeinen in Ordnung zu sagen, dass dieser Code fehlerhaft oder anders ist, oder müssen Sie eine bestimmte Lösung finden?

Keiner der beiden ist ideal IMO. Sprechen Sie am besten mit dem Autor und beheben Sie das Problem gemeinsam.

Code Reviews müssen nicht asynchron sein. Viele Probleme werden gelöst, wenn Sie sie nicht mehr als bürokratischen Prozess ansehen und sich ein wenig Zeit für die Live-Kommunikation nehmen.

guillaume31
quelle
"Bürokratischer Prozess" ist so gut ausgedrückt!
25.
17

Sollte der Prüfer bei Codeüberprüfungen immer eine Lösung für Probleme vorlegen?

Nein. Wenn Sie das tun, sind Sie kein Rezensent, sondern der nächste Programmierer.

Sollte der Prüfer bei Codeüberprüfungen niemals eine Lösung für Probleme vorlegen?

Nein. Ihre Aufgabe ist es, das Problem zu kommunizieren. Wenn die Präsentation einer Lösung das Problem deutlich macht, dann tun Sie es. Erwarten Sie nur nicht, dass ich Ihrer Lösung folge. Das Einzige, was Sie hier tun müssen, ist einen Punkt zu machen. Sie müssen die Implementierung nicht vorschreiben.

Wann sollte ein Prüfer eine Lösung für Probleme vorlegen?

Wenn das die effektivste Art der Kommunikation ist. Wir sind Code-Affen, keine englischen Majors. Manchmal ist der beste Weg, um zu zeigen, dass Code scheiße ist ... weniger als optimal ... es ist, ihnen Code zu zeigen, der weniger scheiße ist ... mehr opt ... oh verdammt, du weißt, was ich meine.

kandierte_orange
quelle
8
Code nicht im luftleeren Raum, denn es ist scheiße.
Hm. Wenn ich eine Lösung für ein Problem vorschlage, hat dies oft Vorteile, die ich kenne, aber es würde einfach zu lange dauern, um eine vollständige Liste von allen zu erstellen. (Dies hängt oft mit der Stabilität zusammen und mit der Gewissheit, dass das Ding weiterarbeitet, während wir andere Dinge ändern.) Wenn Sie also etwas anderes tun, das nicht so viele Probleme löst, wäre ich (zumindest) nicht gerade glücklich es sei denn, Sie könnten mir einen guten Grund nennen, warum das, was ich vorgeschlagen habe, nicht geklappt hat). Wie würden Sie damit umgehen?
jpmc26
1
PS: "Code Monkey" wird oft verwendet, um einen ungeübten, undenkbaren Programmierer zu beschreiben, der einfach das tut, was ihm gesagt wird, auch wenn es eine schlechte Idee ist und kein gutes Designgefühl hat. Siehe Städtisches Wörterbuch . Sogar Wikipedia bemerkt, dass es manchmal abwertend ist.
jpmc26
@ jpmc26 Wenn Sie Code verwenden, um mit mir zu kommunizieren, dann hoffe ich, dass Sie Code verwenden, der zeigt, wie das Problem möglicherweise besser gelöst werden kann. Auch Code Monkey kann mit Zuneigung verwendet werden. Sicherlich mehr Zuneigung als englische Majors bekommen
candied_orange
"Code Affe aufstehen Kaffee bekommen. Code Affe gehen zu Job. Code Affe haben langweilige Treffen mit langweiligen Manager Rob. Rob sagen Code Affe sehr fleißig, aber seine Ausgabe stinken ..."
Baldrickk
13

Das Hauptproblem ist, wenn die Leute wüssten, wie man den Code besser schreibt, hätten sie dies normalerweise zuerst getan. Ob eine Kritik spezifisch genug ist, hängt stark von der Erfahrung des Autors ab. Sehr erfahrene Programmierer könnten eine Kritik wie "Diese Klasse ist zu kompliziert" aufnehmen und zum Zeichenbrett zurückkehren und sich etwas Besseres einfallen lassen, weil sie gerade wegen Kopfschmerzen einen freien Tag hatten oder weil sie schlampig waren, weil sie es waren in Eile.

In der Regel müssen Sie jedoch mindestens die Quelle der Komplikation identifizieren. "Diese Klasse ist zu kompliziert, weil sie überall das Gesetz von Demeter bricht." "Diese Klasse mischt die Verantwortlichkeiten der Präsentationsebene und der Persistenzebene." Zu lernen, diese Gründe zu identifizieren und kurz zu erklären, ist ein Teil davon, ein besserer Prüfer zu werden. Über Lösungen muss man selten ins Detail gehen.

Karl Bielefeldt
quelle
4
+100 Meine häufigste Enttäuschung mit Code Reviews ist, dass ich wahrscheinlich so geschrieben hätte, wenn ich einen besseren Weg gewusst hätte.
RubberDuck
Ich liebe deinen ersten Satz. Ich musste mich fragen: "Ist dieser Code gut genug?" Wirf dann eine Münze, um zu entscheiden, ob du sie verbessern willst oder nicht! (Normalerweise halte ich einfach so lange durch, bis mir die Zeit ausgeht, aber könnte ich vielleicht aufhören, wenn es gut genug ist?)
IMO "Dieser Code ist kompliziert, weil er das Gesetz von Demeter bricht" ist ein schlechter Kommentar. "Dieser Code ist kompliziert, weil X zu stark mit Y und Z gekoppelt ist" ist besser.
immibis
"Wenn die Leute wüssten, wie man den Code besser schreibt, hätten sie dies normalerweise zuerst getan". Es gibt Ausnahmen. Ich habe diesen Code so entwickelt, dass er funktioniert, aber er wird uns irgendwann in den Arsch beißen. Nicht-technischer Manager versteht nicht "Ich mag diesen Code nicht und möchte drei Tage, um ihn zu verbessern". Nicht-technischer Manager versteht "Joe hat diesen Code überprüft und abgelehnt, und ich benötige drei Tage, um ihn zu verbessern".
gnasher729
4

Es gibt zwei Arten von schlechten Programmierern: diejenigen, die nicht den Standardpraktiken folgen, und diejenigen, die "nur" den Standardpraktiken folgen.

Wenn ich nur eingeschränkten Kontakt zur Arbeit hatte / jemandem Feedback gab, würde ich nicht sagen: "Dies ist ein schlechtes Design." aber so etwas wie "Kannst du mir diese Klasse erklären?" Sie werden vielleicht feststellen, dass es eine gute Lösung ist, der Entwickler hat aufrichtig das Beste getan, was er konnte, oder sogar die Erkenntnis, dass es eine schlechte Lösung ist, aber es ist gut genug.

Abhängig von der Reaktion werden Sie eine bessere Vorstellung davon haben, wie Sie sich jeder Situation und Person nähern. Sie können das Problem schnell erkennen und das Problem selbst beheben. Sie können um Hilfe bitten oder versuchen, sie selbst zu lösen.

In unserem Geschäft gibt es Vorschläge für Vorgehensweisen, aber fast alle haben Ausnahmen. Wenn Sie das Projekt verstehen und wissen, wie das Team es angeht, kann dies der Kontext sein, um den Zweck der Codeüberprüfung zu bestimmen und um Bedenken auszuräumen.

Mir ist klar, dass dies eher eine Herangehensweise an das Problem als eine explizite Lösung ist. Es wird zu viel Variabilität geben, um alle Situationen abzudecken.

JeffO
quelle
1
Wenn jedoch eine Erklärung für ein erkennbar gutes Design erforderlich ist, fehlen Inline-Kommentare.
Wildcard
1
Manchmal haben Regeln keine Ausnahmen, aber normalerweise nicht.
@Wildcard - das hängt von den Fähigkeiten und Vorlieben / Meinungen der Leute ab, die es betrachten.
JeffO
1
@Wildcard Ich gehe davon aus, dass das Feedback als Frage formuliert werden sollte, aber die Antwort wird (irgendwann) in Form eines Kommentars oder vielleicht einer Codeänderung (z. B. einer besseren Benennung) erfolgen. Dies lässt dem Entwickler die Tür offen, um sein Denken zu erklären und die Optionen zu diskutieren, anstatt sich wie eine Forderung zu fühlen oder versehentlich seine Arbeit für ihn zu erledigen.
IMSoP
3

Wenn ich den Code überprüfe, gebe ich nur dann eine Lösung für die von mir identifizierten Probleme, wenn ich dies mit geringem Aufwand tun kann. Ich versuche jedoch genau zu sagen, woran das Problem meiner Meinung nach liegt, und greife nach Möglichkeit auf die vorhandene Dokumentation zurück. Die Erwartung, dass ein Prüfer eine Lösung für jedes festgestellte Problem bereitstellt, schafft einen perversen Anreiz - es wird den Prüfer davon abhalten, auf Probleme hinzuweisen.

BagOfSpanners
quelle
3

Meiner Meinung nach wird aus mehreren Gründen in den meisten Fällen auf die Bereitstellung von Code verzichtet:

  • Wenn die Erklärung für sich allein nicht ausreicht, können sie immer eine Probe von dem anfordern, woran Sie denken.
  • Sie verschwenden keine Zeit, indem Sie versuchen, sich mit einem Code vertraut zu machen, den Sie seit langem nicht mehr berührt haben, nur um ihn geringfügig zu ändern, während jemand anderes gerade seine Zeit damit verbracht hat, genau das zu tun.
  • Wenn sie mit dem Code bereits vertraut sind und Sie dies nicht tun, kann es zu einem besseren Code führen, als Sie schreiben würden, wenn Sie nur das Feedback geben. Wenn Sie jemandem eine fertige Lösung geben, wird er diese häufig nur verwenden, ohne darüber nachzudenken, sie weiter zu verbessern.
  • Immer eine Lösung anzubieten, grenzt an Herablassung. Sie arbeiten mit jemandem zusammen, hoffentlich, weil er gut genug war, um eingestellt zu werden. Wenn Sie herausfinden könnten, warum etwas eine schlechte Idee ist, warum würden sie es dann nicht lernen, indem sie auf Feedback hören und es selbst tun?
  • Es ist einfach komisch, immer eine Lösung anzubieten. Stellen Sie sich vor, Sie programmieren ein Paar an ihrem Schreibtisch: Sie haben gerade ein paar Zeilen beendet, die Sie für nicht gut halten. Erzählst du ihnen einfach, was du entdeckt hast und warum, oder greifst du nach ihrer Tastatur und zeigst sofort deine Version? So kann es sich für andere Menschen anfühlen, wenn Sie immer Ihre Lösung bereitstellen.
  • Sie können immer sagen, was Sie stattdessen tun würden, ohne die Zeit dafür aufzuwenden, es tatsächlich zu schreiben. Sie haben genau das getan, als Sie das erste Problem in der Frage beschrieben haben.
  • Verteile kein Essen, lehre das Fischen;)

Sicher, es gibt einige Fälle, in denen Sie an eine bestimmte Alternative denken, und es lohnt sich, sie beizufügen. Aber das ist meiner Erfahrung nach sehr selten. (viele Rezensionen, große Projekte) Der Originalautor kann Sie jederzeit nach einem Muster fragen, wenn dies erforderlich ist.

Selbst dann kann es aus dem dritten Grund sinnvoll sein, bei der Abgabe eines Beispiels zu sagen, dass die Verwendung x.foo()dieses Beispiels einfacher ist als eine vollständige Lösung. Lassen Sie den Autor dies schreiben. Dies spart auch Ihre Zeit.

viraptor
quelle
Ihr fünfter Punkt brachte mich zum Lächeln. Ich stellte mir vor, wie man sich mit Tastaturen duelliert, um zu sehen, wer zuerst eine großartige Lösung finden könnte. Wer hätte gedacht, dass Pair Programming wie diese beiden Rennauto-Arcade-Spiele oder ein Full-Contact-Sport sein könnte? " Steve hat Ron brutal in das BSOD eingecheckt, 2 Minuten im Strafraum ... "
2

Ich denke, der Schlüssel zu Code-Überprüfungen besteht darin, sich vor der Überprüfung auf die Regeln zu einigen.

Wenn Sie klare Regeln haben, sollte es nicht nötig sein, eine Lösung anzubieten. Sie überprüfen nur, ob die Regeln befolgt wurden.

Das einzige Mal, wenn die Frage nach einer Alternative auftaucht, ist, ob das ursprüngliche Gerät nicht über eine Möglichkeit nachdenkt, die Funktion zu implementieren, die den Regeln entspricht. Angenommen, Sie haben eine Leistungsanforderung, aber die Funktion dauert nach mehreren Optimierungsversuchen länger als Ihr Schwellenwert.

Jedoch! wenn deine Regeln subjektiv sind "Namen müssen von mir genehmigt werden!" dann, ja, Sie haben sich gerade zum Chef ernannt und sollten Anfragen nach Listen akzeptabler Namen erwarten.

Ihr Vererbungsbeispiel (optionale Parameter) ist vielleicht besser, da ich Regeln zur Codeüberprüfung gesehen habe, die lange Methoden und "zu viele" Funktionsparameter verbieten. Normalerweise können diese jedoch durch Aufteilen trivial gelöst werden. Es ist der Teil "Diese Lösung schien die Dinge zu komplizieren", in dem Ihre Objektivität stört und möglicherweise eine Begründung oder eine alternative Lösung erfordert.

Ewan
quelle
2
"Ich denke, der Schlüssel zu Code-Überprüfungen ist, sich vor der Überprüfung auf die Regeln zu einigen." Dies wäre der Idealfall. In der Praxis können wir nicht davon ausgehen, dass jeder Entwickler alle Regeln kennt. Code Reviews sind nützlich, um dieses Wissen zu verbreiten und die Regeln mit praktischen Beispielen zu erklären. Vielleicht ist das einer der größten Vorteile von Code-Reviews.
Frank Puffer
Schreiben Sie die Regeln in das Dokument mit den Kodierungsstandards und geben Sie sie an neue Entwickler weiter
Ewan,
1
Wir haben Codierungsstandards aufgeschrieben, die neuen Entwicklern zur Verfügung gestellt werden. Dies funktioniert meistens, aber manchmal gibt es Fehlinterpretationen. Zusätzlich zu den niedergeschriebenen Codierungsstandards gibt es allgemeine Prinzipien wie DRY oder SOLID, die auch in Code Reviews behandelt werden. Wir erwarten von unseren Entwicklern ein Grundwissen darüber und führen auch einige interne Schulungen durch, um es zu verbessern. Dies ist ein fortlaufender Prozess und Codeüberprüfungen sind Teil davon.
Frank Puffer
0

Wenn sich eine mögliche Lösung schnell und einfach eintippen lässt, versuche ich, sie in meine Peer-Review-Kommentare aufzunehmen. Wenn nicht, schreibe ich wenigstens mein Anliegen auf und warum finde ich diesen bestimmten Punkt problematisch. Bei Variablen- / Funktionsnamen, bei denen Sie sich nichts Besseres vorstellen können, erkenne ich normalerweise an, dass ich keine bessere Idee habe, und beende den Kommentar in Form einer offenen Frage, falls dies jemand kann . Auf diese Weise wird die Überprüfung nicht wirklich aufgehalten, wenn sich niemand eine bessere Option einfallen lässt.

Für das Beispiel, das Sie in Ihrer Frage gegeben haben, mit der schlecht gestalteten Klasse. Ich möchte einige Kommentare hinterlassen, die besagen, dass Vererbung wahrscheinlich der beste Weg ist, um das Problem zu lösen, das der Code zu lösen versucht, und es dabei belassen. Ich würde versuchen, in einer Weise zu formulieren, die anzeigt, dass es kein Show-Stopper ist und es im Ermessen des Entwicklers liegt, ob es behoben werden soll oder nicht. Ich würde auch mit der Bestätigung eröffnen, dass Sie mit diesem Teil des Codes nicht besonders vertraut sind, und sachkundigere Entwickler und / oder Prüfer einladen, um zu klären, ob es einen Grund gibt, warum es so gemacht wird.

Eric Hydrick
quelle
0

Gehen Sie und sprechen Sie mit der Person, deren Code Sie überprüfen. Sagen Sie ihnen freundlich, dass es Ihnen schwer gefallen ist, das zu verstehen, und besprechen Sie dann mit ihnen, wie es klarer formuliert werden könnte.

Schriftliche Kommunikation führt zu einer enormen Zeitverschwendung sowie zu Ressentiments und Missverständnissen.

Von Angesicht zu Angesicht ist die Bandbreite viel höher und man hat den emotionalen Seitenkanal, um Feindseligkeiten vorzubeugen.

Wenn Sie tatsächlich mit dem Jungen sprechen, geht das viel schneller, und Sie können einen neuen Freund finden und feststellen, dass Sie beide mehr Spaß an Ihrer Arbeit haben.

John Lawrence Aspden
quelle
dies scheint nicht alles zu bieten erhebliche über Punkte gemacht und erläutert vor 11 Antworten
gnat