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

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?

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.