Is a code review which uses only code comments a good idea?

Question

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.

Questions

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

Answer 1

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. ;)

Answer 2

I would supplement the comments in the code with a companion document. This would summarize the findings and live on after the comments were removed. The advantages of this would be:

• compactness. If the person routinely fails to check that a pointer passed in isn't null (or some other common beginner error in the language you're using) the reviewer can leave dozens of REV comments to that effect, and in the document can say "I found 37 places where pointers were not checked first" without listing them all
• place for praise. A comment like REV this is a nice design just seems kind of odd, but my code reviews often include approval as well as corrections
• interactivity. Your sample comment is "why did you do this?" and it's a very typical one. A comments-only approach doesn't support a conversation. The person can change their code and delete the comment, or delete the comment. But "why did you do this?" and "please change this, it's wrong" are not the same thing.
• keeping a record. A later reviewer can check whether the code was actually changed, or the comments were just removed. They can also check that the comments have been "taken on board" and the developer is no longer making the same mistakes in a subsequent review.

I would also use a work item for doing the review and responding to the review, and associate the checkins with it. That makes it easy to find the comments in an old changeset, and to see how each was handled in another changeset.

Answer 3

Others have talked about of the limitations of this approach. You mentioned that tools like gerrit were hard to install - I suggest you take a look at phabricator (http://www.phabricator.com). This is the code review system that Facebook has used for years, and was recently open sourced. It's not hard to install, has excellent workflows, and solves all of the issues mentioned in the other comments.

Answer 4

This should be a comment to Arseni's answer... But I don't have enough credit to comment.

I am familiar with this approach and I believe it's superior to reviews that rely on external collaboration tools. The reason why I believe this approach is superior is because the reviewer comments are as close to the code as it possibly can be. And the coder does not need to switch between code and some tool while addressing reviewer's comments.

I am aware of similar process working on big scale (not a small team).

The improvement required for the process to work on a big scale.

When developer removes the "// REV" comment the reviewer can actually see it because reviewer compares new changes against his version (version containing his comments). For that you might need to create a simple tool (I have a bash script) that keeps track of what is being reviewed and where you are in a review. (with git I used git worktrees so that it does not impact my other work) Another option (simpler) is for developer to never remove the comments but rather marked them as addressed "// ADDRESSED-REV" this way it's like a 3-way hand shake where the final say is for reviewer who, when satisfied, removes the comment. In addition you can add REV from @someperson to know exactly who's comment is that. This way reviewer can ignore other people comments on each iteration.

Another important thing is to introduce some hind of "merge" hook that prevents merge where code contains "REV" or "ADDRESSED-REV" comments. This prevents unaddressed issues.

The REV, ADDRESSED-REV, REV, ADDRESSED-REV etc. discussions can be later converted into // After discussion we decided to do this because of this comment or could be removed altogether if the code is obvious. This way reviewer on each iteration can concentrate on her/his comments.