Ist eine Codeüberprüfung, bei der nur Codekommentare verwendet werden, eine gute Idee?

16

Voraussetzungen

  • Team nutzt DVCS
  • IDE unterstützt das Parsen von Kommentaren (wie TODO usw.)
  • Tools wie CodeCollaborator sind teuer fürs Budget
  • Tools wie gerrit sind zu komplex für die Installation oder nicht verwendbar

Arbeitsablauf

  • Der Autor veröffentlicht irgendwo auf der zentralen Repo-Zweigstelle
  • Prüfer holen es und starten die Überprüfung
  • Im Falle einer Frage / eines Problems können Sie einen Kommentar mit einem speziellen Label wie "REV" erstellen. Ein solches Etikett DARF NICHT im Produktionscode enthalten sein - nur in der Überprüfungsphase:

    $somevar = 123;
    // REV Why do echo this here?
    echo $somevar;
    
  • Wenn der Rezensent mit dem Posten von Kommentaren fertig ist, wird nur die dumme Meldung "Kommentare" ausgegeben und zurückgeschoben

  • Der Autor zieht den Feature-Zweig zurück und beantwortet Kommentare auf ähnliche Weise oder verbessert den Code und schiebt ihn zurück
  • Wenn "REV" -Kommentare weg sind, können wir annehmen, dass diese Überprüfung erfolgreich abgeschlossen wurde.
  • Der Autor baut den Feature-Zweig interaktiv neu auf, drückt ihn zusammen, um diese "Kommentar" -Eingaben zu entfernen, und ist nun bereit, das Feature zusammenzuführen, um eine Aktion zu entwickeln oder durchzuführen, die nach erfolgreicher interner Überprüfung normalerweise erfolgen könnte

IDE-Unterstützung

Ich weiß, dass benutzerdefinierte Kommentar-Tags in Eclipse & Netbeans möglich sind. Klar sollte es auch in der blablaStorm Familie sein.

Beispiel für benutzerdefinierte gefilterte Aufgaben aus Kommentaren in Eclipse

Fragen

  1. Halten Sie diese Methodik für sinnvoll?
  2. Kennst du etwas ähnliches?
  3. Was kann daran verbessert werden?
gaRex
quelle
Gute Frage, aber es hat nichts mit Servietten zu tun - kein großartiger Titel.
MarkJ
@MarkJ wie soll ich es dann nennen? Ich meine etwas Handwerkliches, Hausgemachtes. Auf Russisch haben wir Phrase "на коленке". Etwas nicht stabiles, nicht ideales, nicht festes wie, aber das funktioniert.
gaRex
2
@MarkJ, gaRex: Was ist mit diesem neuen Titel? Sie können jederzeit zurückkehren, wenn Sie der Meinung sind, dass dies für diese Frage nicht geeignet ist.
Arseni Mourzenko
@ MainMa, es ist in
Ordnung
1
Atlassian Crucible ist für bis zu 10 Entwickler im Wesentlichen kostenlos. Es ist auch das beste Tool zur Codeüberprüfung, das ich je verwendet habe. Der Kommentaransatz ist sinnvoll, kann es jedoch schwierig machen, den Status zu verfolgen.
Andrew T Finnell

Antworten:

14

Die Idee ist eigentlich sehr schön. Im Gegensatz zu herkömmlichen Workflows wird die Überprüfung direkt im Code gespeichert. Technisch gesehen benötigen Sie also nur einen Texteditor , um diesen Workflow zu verwenden. Die Unterstützung in der IDE ist auch nett, insbesondere die Möglichkeit, die Liste der Bewertungen unten anzuzeigen.

Es gibt noch ein paar Nachteile:

  • Es funktioniert gut für sehr kleine Teams, aber größere Teams müssen nachverfolgen, was wann von wem mit welchem ​​Ergebnis überprüft wurde. Obwohl Sie über diese Art der Verfolgung verfügen (die Versionskontrolle ermöglicht es, all das zu wissen), ist die Verwendung und Suche äußerst schwierig und erfordert häufig eine manuelle oder halbmanuelle Suche durch die Revisionen.

  • Ich glaube nicht, dass der Prüfer genug Feedback vom Prüfer hat, um zu wissen, wie die geprüften Punkte tatsächlich angewendet wurden .

    Stellen Sie sich folgende Situation vor. Alice überprüft zum ersten Mal den Code von Eric. Sie bemerkt, dass Eric, ein junger Entwickler, die Syntax verwendet hat, die in der tatsächlich verwendeten Programmiersprache nicht die aussagekräftigste ist.

    Alice schlägt eine alternative Syntax vor und sendet den Code an Eric zurück. Er schreibt den Code mit dieser alternativen Syntax neu, die er für richtig hält, und entfernt den entsprechenden // BLAKommentar.

    In der nächsten Woche erhält sie den Code für die zweite Überprüfung. Würde sie sich tatsächlich daran erinnern können, dass sie diese Bemerkung bei ihrer ersten Überprüfung gemacht hatte, um zu sehen, wie Eric sie löste?

    In einem formaleren Überprüfungsprozess konnte Alice sofort feststellen, dass sie eine Bemerkung machte und den Unterschied des relevanten Codes sah, um festzustellen, dass Eric die Syntax, von der sie ihm erzählte, falsch verstanden hatte.

  • Menschen sind immer noch Menschen. Ich bin mir ziemlich sicher, dass einige dieser Kommentare im Produktionscode landen und andere als Müll bleiben, obwohl sie völlig veraltet sind .

    Das gleiche Problem besteht natürlich auch bei anderen Kommentaren. Beispielsweise befinden sich viele TODO-Kommentare in der Produktion (einschließlich des nützlichsten: "TODO: Kommentieren Sie den folgenden Code.") und viele Kommentare, die nicht aktualisiert wurden, als der entsprechende Code vorhanden war.

    Beispielsweise kann der ursprüngliche Autor des Codes oder der Rezensent den Code verlassen, und der neue Entwickler würde nicht verstehen, was in der Rezension steht, so dass der Kommentar für immer erhalten bleibt und darauf wartet, dass jemand zu mutig wäre, ihn zu vernichten oder tatsächlich zu verstehen, was er tut es sagt.

  • Dies ersetzt keine persönliche Überprüfung (das gleiche Problem tritt jedoch auch bei anderen formellen Überprüfungen auf, die nicht von Angesicht zu Angesicht durchgeführt werden).

    Insbesondere, wenn für die ursprüngliche Überprüfung eine Erklärung erforderlich ist, beginnen der Überprüfer und der Überprüfende eine Art Diskussion . Sie werden nicht nur mit großen BLA-Kommentaren konfrontiert sein, sondern diese Diskussionen werden auch das Protokoll der Versionskontrolle verschmutzen .

  • Möglicherweise treten auch kleinere Probleme mit der Syntax auf (die auch für TODO-Kommentare vorhanden ist). Was ist zum Beispiel, wenn ein langer "// BLA" -Kommentar in mehreren Zeilen erscheint und jemand beschließt, ihn so zu schreiben:

    // BLA This is a very long comment which is way beyond 80 characters, so it actually
    // fills more then one line. Since the next lines start with slashes, but not "BLA"
    // keyword, the IDE may not be able to show those lines, and display only the first one.
    
  • Und zum Schluss als kleine und sehr persönliche Anmerkung: Wählen Sie nicht "BLA" als Schlüsselwort. Es klingt hässlich. ;)

Arseni Mourzenko
quelle
"um zu wissen, wie die bewerteten Punkte tatsächlich angewendet wurden" Ja :) Nur Ehrlichkeit des Rezensenten. Hier haben wir mehr Politik als Werkzeug.
gaRex
1
"Menschen sind Menschen" Ja :( Wir haben bereits Millionen dieser TODOs. Kann es sein, dass diese Funktion nicht in IDEs enthalten ist?
gaRex
"Verschmutzen Sie das Protokoll der Versionskontrolle" Dafür befindet sich die gesamte Arbeit in einem eigenständigen Feature-Zweig, der später in der Entwicklung zusammengeführt wird. Möglicherweise könnte dies durch einfaches Einhängen von DVCS geschehen. Aber wie ich sehe, verschiebt sich die Komplexität von Code-Review-Tools zu IDEs und DVCS.
gaRex
"kleinere Probleme mit der Syntax" Kann es sein, dass es kein Problem ist? Diese REVs sind nur als einige Markierungen, um schnell vom Panel darauf zuzugreifen.
gaRex
1
@gaRex: Dann sollten Ihre Teammitglieder eine E-Mail verwenden, um ein persönliches Rendevouz-Datum zu vereinbaren ;-)
Doc Brown
4

Ich würde die Kommentare im Code mit einem Begleitdokument ergänzen. Dies würde die Ergebnisse zusammenfassen und weiterleben, nachdem die Kommentare entfernt wurden. Die Vorteile davon wären:

  • Kompaktheit. Wenn die Person routinemäßig nicht überprüft, ob ein übergebener Zeiger nicht null ist (oder ein anderer häufiger Anfängerfehler in der von Ihnen verwendeten Sprache), kann der Prüfer Dutzende von REV-Kommentaren zu diesem Zweck hinterlassen und im Dokument Folgendes angeben: Ich habe 37 Stellen gefunden, an denen Zeiger nicht zuerst überprüft wurden ", ohne sie alle aufzulisten
  • Platz für Lob. Ein Kommentar wie dieser REV this is a nice designscheint seltsam zu sein, aber meine Code-Überprüfungen enthalten häufig sowohl Genehmigungen als auch Korrekturen
  • Interaktivität. Ihr Beispielkommentar lautet "Warum haben Sie das getan?" und es ist ein sehr typisches. Ein Ansatz nur mit Kommentaren unterstützt keine Konversation. Die Person kann ihren Code ändern und den Kommentar löschen oder den Kommentar löschen. Aber "warum hast du das getan?" und "bitte ändere das, es ist falsch" sind nicht dasselbe.
  • ein Protokoll führen. Ein späterer Prüfer kann überprüfen, ob der Code tatsächlich geändert oder die Kommentare nur entfernt wurden. Sie können auch überprüfen, ob die Kommentare "berücksichtigt" wurden und der Entwickler in einer nachfolgenden Überprüfung nicht mehr dieselben Fehler macht.

Ich würde auch ein Arbeitselement verwenden, um die Überprüfung durchzuführen und auf die Überprüfung zu antworten, und die Eincheckvorgänge damit verknüpfen. Das macht es einfach, die Kommentare in einem alten Änderungssatz zu finden und zu sehen, wie jeder in einem anderen Änderungssatz behandelt wurde.

Kate Gregory
quelle
dann brauchen wir ein gutes Code-Review-Tool. Unser derzeit beschriebener Ansatz ist hauptsächlich politisch, da die Leute ein Ethiquet erfinden und ihm folgen sollten.
gaRex
"Kompaktheit" - Ich denke, es könnte durch einen Kommentar wie "// REV Alter, wir haben 37 Stellen, an denen Zeiger nicht zuerst überprüft wurden, einschließlich dieser. Bitte RTFM"
gaRex
"Platz für Lob" - auch möglich, aber nach dem Quetschen wird verloren gehen :( Ich sehe bereits ein Problem - wir haben die Bewertungshistorie beim Quetschen verloren.
gaRex
"Interaktivität" - Der Autor kann in einem anderen Kommentar unter dem Anfangsbuchstaben antworten. Genau wie im Wikipedia-Stil.
gaRex
"Person kann ... den Kommentar löschen" - es ist ein Problem. +1
gaRex
2

Andere haben über die Grenzen dieses Ansatzes gesprochen. Sie haben erwähnt, dass Tools wie gerrit schwer zu installieren sind - ich schlage vor, Sie werfen einen Blick auf phabricator (http://www.phabricator.com). Dies ist das Codeüberprüfungssystem, das Facebook seit Jahren verwendet und das vor kurzem als Open-Source-Lösung angeboten wurde. Es ist nicht schwer zu installieren, verfügt über hervorragende Arbeitsabläufe und behebt alle in den anderen Kommentaren genannten Probleme.

Adam Hupp
quelle
Beeindruckend! Wir müssen es versuchen, anstatt unserer schönen Redmine.
gaRex
"gerrit" Ich habe es installiert - es war nicht so schwer. Ich benutze es einfach nicht! Und es hat auch hässliche Benutzeroberfläche und Workflow.
gaRex
@gaRex Wir verwenden Reviewboard ( reviewboard.org ) parallel zu Redmine. Sie dienen unterschiedlichen Zwecken, also ist das in Ordnung. Und Sie können RB einrichten, um auf Redmine-Themen zu verlinken ...
Johannes S.
@JohannesS. welche vcs benutzt du Haben Sie auch ein fertiges Howto oder Wiki zu deren Integration? Schön, so einen zu haben.
gaRex
@gaRex Entschuldigung für die verspätete Antwort. Wir verwenden SVN, aber RB unterstützt auch git (tatsächlich verwenden die RB-Leute git selbst). Ich schlage vor, einen Blick auf die Website von RB zu werfen. Es ist eine Online-Demo verfügbar (z. B. check out demo.reviewboard.org/r/101 )
Johannes S.