Code Reviews – Handling Disagreement on Edge Cases

We used to do manual code reviews (i.e. no special tools) and this was the best method we found. Our development department doesn't have team foundation server, so I can't comment on that.

1. All work is tied to a bug or an agile story that has a unique ID. When the work is checked in (or shelved, which is submission without actual check in), the description will always have this identifier.
2. In a stand-up or over e-mail the reviewer would be notified that bug ### or story ### is ready for review.
3. They do source control diff between new versions of the files and previous versions and copy/paste the output of diff into a word document. We have a code review word document template which is preset to use have 2 levels of headings: level 1-module, level 2-file name. It also defines formatting style for actual code (8pt Consolas). This step obviously was the biggest pain, but with Word template and Ctrl-A, Ctrl-C, Ctrl-V sequence, it's not that many clicks.
4. Now that the document is in word, the reviewer can highlight the code in question and add comments using Word's review system.
5. All documents are stored in a document repository and labeled with corresponding bug or story number.
6. Once the document is done, the link to the repository is shared with original developer. Then we'd either have a code review meeting if changes were significant enough and need discussion, or just leave it up to original developer to go through comments on his own time.

After trying numerous manual methods, this was the one we found to have the least amount of overhead while allowing us to actually review every change.

However, our engineering team just rolled out Review Board and although I haven't used it that much yet, so far I'm loving what I have seen. It has all the flexibility of our word docs, but no more copy/pasting and manually fixing formatting. As an added bonus it keeps a permanent archive of all comments so you can go back years in time if you ever need to. Also it allows you do diffs of diffs, which is great when you want to do a review of a code review. This part we found to be very difficult with manual procedures because you can't see what was changed in response to first code review, instead you end up redoing the entire thing.

Although you did say you don't want to use tools, I'd strongly urge you to consider Review Board. It is open source and completely free. So you can roll it out for yourself and the 5 people you maybe working with. The rest of your company doesn't have to use this tool if they don't want to. And you don't need to worry about getting any purchase approvals.

== Update to comments from question: ==

On my team, we have people in NY, CT, TX, Poland and India. What makes things even more interesting is that extremely high percentage of the team doesn't know the product or technology all that well, so very few of us do most of the review. So yeah, senior devs are definitely busy. In the process I outlined, primary reviewer would do initial walk through the code independently on his own schedule. But afterwards we schedule a meeting and primary reviewer walks the coder through his comments. The meeting can have other reviewers, but they are considered secondary and are not obligated (but not discouraged from) to review every file or make comments.

I agree with others comments that having the final meeting in real-time, even if you have to use web conferencing is much better for knowledge transfer and for helping your new guys understand the code so that they'll start producing things that make your senior devs even busier. But again, that depends on the volume and type of comments, sometimes (very infrequently) comments are so minor that the meeting is skipped.

I can also relate to you when you say it's hard to roll out new tools. I work for a very large corporation and typically there's so many people involved that even outside of the fact that no purchases are ever approved, there are so many interests/agendas that nothing is ever agreed on. What's nice about Review Board is that you can skip all that and just start using with your small team and if you have to (if it really comes to that), you can host the web services on your own dev machine.

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