Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?
share|improve this question
    
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. –  MainMa 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. –  Andrew Finnell Oct 4 '12 at 23:24

3 Answers 3

up vote 14 down vote accepted

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

share|improve this answer
    
"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

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.

share|improve this answer
    
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

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.

share|improve this answer
    
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... –  Johannes S. 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) –  Johannes S. Nov 9 '12 at 9:22

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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