Code Reviews – Is Using Only Code Comments Effective?

code-reviewsdvcsteamworkworkflows

Preconditions

  • Team uses DVCS
  • IDE supports comments parsing (like TODO and etc.)
  • Tools like CodeCollaborator are expensive for budget
  • Tools like gerrit are too complex for install or not usable

Workflow

  • Author publishes somewhere on central repo feature branch
  • Reviewer fetch it and start review

  • In case of some question/issue reviewer create comment with special label, like "REV". Such label MUST not be in production code — only on review stage:

    $somevar = 123;
// REV Why do echo this here?
echo $somevar;

  • When reviewer finish post comments — it just commits with stupid message "comments" and pushes back

  • Author pulls feature branch back and answer comments in similar way or improve code and push it back
  • When "REV" comments have gone we can think, that review has successfully finished.
  • Author interactively rebases feature branch, squashes it to remove those "comment" commits and now is ready to merge feature to develop or make any action that usualy could be after successful internal review

IDE support

I know, that custom comment tags are possible in eclipse & netbeans. Sure it also should be in blablaStorm family.

Example of custom filtered task-from-comments in eclipse

Questions

  1. Do you think this methodology is viable?
  2. Do you know something similar?
  3. What can be improved in it?

Best Answer

The idea is actually very nice. Contrary to common workflows, you keep the review directly in code, so technically, you don't need anything but text editor to use this workflow. The support in the IDE is nice too, especially the ability to display the list of reviews in the bottom.

There are still a few drawbacks:

  • It works fine for very small teams, but larger teams will require tracking over what were reviewed, when, by whom and with which result. While you actually have this sort of tracking (version control allows to know all that), it's extremely difficult to use and search, and will often require a manual or semi-manual search through the revisions.

  • I don't believe that the reviewer has enough feedback from the reviewee to know how the reviewed points were actually applied.

    Imagine the following situation. Alice is reviewing for the first time the code of Eric. She notices that Eric, a young developer, used the syntax which is not the most descriptive in the programming language actually used.

    Alice suggests an alternative syntax, and submits the code back to Eric. He rewrites the code using this alternative syntax that he believes understanding correctly, and removes the corresponding // BLA comment.

    The next week, she receives the code for the second review. Would she be able to actually remember that she made this remark during her first review, in order to see how Eric solved it?

    In a more formal review process, Alice could immediately see that she made a remark, and see the diff of the relevant code, in order to notice that Eric misunderstood the syntax she told him about.

  • People are still people. I'm pretty sure that some of those comments will end up in production code, and some will remain as a garbage while being completely outdated.

    Of course, the same problem exists with any other comment; for example there are lots of TODO comments in production (including the most useful one: "TODO: Comment the code below."), and lots of comments which were not updated when the corresponding code was.

    For example, the original author of the code or the reviewer may leave, and the new developer would not understand what the review says, so the comment will remain forever, awaiting that somebody would be too courageous to wipe it out or to actually understand what it says.

  • This does not replace a face-to-face review (but the same problem applies as well to any other more formal review which is not done face-to-face).

    Especially, if the original review requires explanation, the reviewer and the reviewee will start a sort of a discussion. Not only you will find yourself with large BLA comments, but those discussions will also pollute the log of the version control.

  • You may also encounter minor issues with the syntax (which also exists for TODO comments). For example, what if a long "// BLA" comment spawns on several lines, and somebody decides to write it this way:

    // 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.

  • And finally as a minor and very personal note: don't choose "BLA" as a keyword. It sounds ugly. ;)

Related Topic