17

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?
14
  • Good question but it doesn't have anything to do with napkins - not a great title.
    – MarkJ
    Oct 4 '12 at 7:10
  • @MarkJ how to name it then? I mean somthing handicraft, cottage, home-made. In Russian we have phrase "на коленке". Something not stable, not ideal, non-solid like, but that works.
    – gaRex
    Oct 4 '12 at 7:20
  • 2
    @MarkJ, gaRex: what about this new title? Feel free to revert if you find it not appropriate for this question. Oct 4 '12 at 7:59
  • @MainMa, it's ok
    – gaRex
    Oct 4 '12 at 9:53
  • 1
    Atlassian Crucible is essentially free for up to 10 developers. It also happens to be the best code review tool I've used. The comments approach is viable but can make it hard to track state. Oct 4 '12 at 23:24
15

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

10
  • "to know how the reviewed points were actually applied" Yes :) Only honesty of reviewee. Here we have more policy, than tool.
    – gaRex
    Oct 4 '12 at 4:40
  • 1
    "people are people" Yes :( We already have millions of these TODOs. May be just deny to have such feature in IDEs?
    – gaRex
    Oct 4 '12 at 4:42
  • "pollute the log of the version control" For this all work is in standalone feature branch, that later being squashed & merged in develop. May be this could be done by simple hook in DVCS. But as I see all complexness moves from code-review tools to IDEs & DVCS.
    – gaRex
    Oct 4 '12 at 4:44
  • "minor issues with the syntax" May be it's not issue? Those REV`s are just only as some markers to quickly go to it from panel.
    – gaRex
    Oct 4 '12 at 4:47
  • 1
    @gaRex: then your team members should use email to agree about a face-to-face rendevouz date ;-)
    – Doc Brown
    Oct 4 '12 at 21:10
4

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.

6
  • then we need some good code-review tool. Our current described approach is mostly political as people should invent some ethiquet and follow it.
    – gaRex
    Oct 4 '12 at 13:41
  • "compactness" -- I think it could be done by one comment like "// REV Dude, we have 37 places where pointers were not checked first, including this one. Please RTFM"
    – gaRex
    Oct 4 '12 at 13:42
  • "place for praise" -- also possible, but after squashing will be lost :( I already see one issue -- we lost review history when squashing commits.
    – gaRex
    Oct 4 '12 at 13:43
  • "interactivity" -- author can answer in another comments, below initial. Just like in wikipedia style.
    – gaRex
    Oct 4 '12 at 13:44
  • "person can ... delete the comment" -- it's an issue. +1
    – gaRex
    Oct 4 '12 at 13:44
2

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.

5
  • wow! We need to try it instead of our lovely redmine.
    – gaRex
    Oct 5 '12 at 1:07
  • "gerrit" I installed it -- it wasnt so hard. I just fail to use it! And also it has ugly UI and workflow.
    – gaRex
    Oct 5 '12 at 1:09
  • @gaRex We use Reviewboard (reviewboard.org) in parallel with Redmine. They serve different purposes so that' s just fine. And you can setup RB to link to Redmine issues... Oct 16 '12 at 11:57
  • @JohannesS. which vcs do you using? Also do you have some ready howto or wiki about their integration? Nice to have such one.
    – gaRex
    Oct 16 '12 at 16:31
  • @gaRex Sorry for late reply. We use SVN, but RB supports git as well (actually, the RB people use git themselves). I suggest to have a look at RB's website. There is an online demo available (e.g. check out demo.reviewboard.org/r/101) Nov 9 '12 at 9:22
1

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.