Hey Brad I’ve worked on a few different software development teams some did code reviews and some did not. Those that did all did them differently. I ask you what the heck is a code review and why should I do them?

First off I think Code Review is a good thing don’t get me wrong; I agree that they should be done for two reasons:

1. The activity promotes shared knowledge; The code base is being worked on by multiple developers and personally I don’t generally have the time to just sit down and comb the code base to see how its changing over time. Code reviewing does allow me the chance to see what changes are occurring outside of my own task list. On projects I’ve worked on however we fell into a pattern of generating experts in different parts of the code where as if John didn’t do the change related to Module A then he definitely would be the sole code reviewer; I’d recommend spreading the love around and not to elect experts to different components of the code to be the go to reviewer. Pick random members of the team (we’re all experts in our field) so that team members who tend to gravitate to Module B get to see what’s going on in Module A – Shared Knowledge.
2. It promotes adherence to the design; In lieu of a written down design allowing more eyes on every code change helps the team be more aware of the overall structure of the code. As a fictitious example if I just reviewed a code change by Abdul for Module A and he, I don’t know, created an XML parser class that uses Qt and put it in a common lib (let’s say) if I then see a code change come in by Randy for Module B and he too made an XML parser class but using Xerces and put it in a serialization lib it would prompt me to grab the two of them and have a conversation where we collectively agree upon which third party lib to use and where to put it therefore generating a common structure where none existed before. Without Code Reviews that duplication most likely would be missed for some time and the conversation around design never occurs.

Notice that my above reasons say nothing about policing coding standard / style guide lines. That is because a code review should be focused on the design and structure of the code not if your using proper spacing and position of curly brackets; we have tools to help enforce that ( like Artistic Style for example ). A Code Review shouldn’t get held up or rejected because someone forgot to hit the space bar between a brace and a parameter name.

Code Reviews (in my opinion) should focus on:

• Is the code safe?
• Is the code efficient?
• Is the code sensible?
• Is the code reusable?
• Does the code adequately achieve the purpose of the change?
• Does the code have sufficient tests?

Post-Commit or Pre-Commit Review?

In the world of Code Reviews there are two flavors, Pre-Commit and Post-Commit. Do I submit my code review request before or after I commit my code changes to the main line?

Pre-Commit

There are nice tools for facilitating pre-commit code reviews like Review Board, which is a WUI that lets you upload a patch for people to review (saving them having to check out a clean copy of the code base and apply your patch) and allows reviewers to make inline comments right in the diffs saving you having to look up line 147 to see what they are talking about.

The only Conn I have for Pre-Commit Reviews is that it prevents you from making spot check-ins that would have: (a) given you history of what it was you were doing during development, (b) saved you from fixing yourself to death, and (c) hinders join development between multiple developers.

For me I want to be able to make as many commits as I feel sane; if what I’m modifying is basically inert to the project I should be allowed to commit it. Of course I would not advocate for allowing someone to commit incomplete changes that brake existing functionality but if a partial commit won’t affect the current state of the application I say go nuts you can always revert it later if it doesn’t work out.

Post-Commit

Post-Commit code reviews can be a bit more tricky as I’m not aware of too many tools that can parse the repository for all the relevant commits to generate a patch for you (not sure if there is any really, sounds like a tall order) but if you were working in a developer branch then you could patch your main-line merge and submit that for review using the same tools you would have used for Pre-Commit Code Reviews (i.e. ReviewBoard for example). This of course means your working in a branch so you now have to deal with branch management and there will be a merging over head (Revers Integration (RI) and Forward Integration (FI) to keep your branch and the main line in sync.

On projects I’ve worked on where we didn’t want to pay the overhead of branch management (unless it was a really big experimental change in-which case we spiked it) we done without tools and simply emailed each other patches, revision numbers, or if we’re adding a new file just said check out this new file please. Not as nice as having a fancy tool that lets me do inline comments and web based diffs but still manageable although can be time consuming (sifting through multiple revisions) and error pron (i.e. oh I forgot about that commit as this task took 5 days to complete).

Personally until such time as I can find a tool for a centralized repository that makes generating a patch from multiple commits (ideally without me having to remember each one) I’m much more in favor for the probably better named Post-Commit-ish process of working in a developer branch (one-to-one developer to branch or a shared branch for joint development), committing changes all day long then when the feature is completed and ready for QA merge it into the main-line. At that point do a single patch of your merge and submit a Pre-Commit Code Review via a tool like the above mentioned ReviewBoard.
 
This has just been my thoughts on what the heck a Code Review is; a way to ensure code quality, adherence to design and share knowledge around the team. If you have a different view or have any comments or questions feel free to leave them below and I’ll try to answer them as time permits.

Until next time think imaginatively and design creatively