What will be the best way to manage reviewed source code in a source control repository? Should the source code go through a review process before getting checked in, or should the code review happen after the code is committed? If the review happens after the code is checked in to the repository, then how should that be tracked?
Google has the best code review practices of any place I have ever seen. Everyone I met there is in complete agreement on how to do code reviews. The mantra is "review early and often".
Suppose you use a process that looks like what Graham Lee suggested. (Which is a process I'd previously used myself.) The problem is that reviewers are being asked to look at big chunks of code. That is a lot more effort, and it is harder to get reviewers to do it. And when they do do it, it is harder to get them to do a thorough job of it. Furthermore when they notice design issues, it is harder to get developers to go back and redo all of their working code to make it better. You still catch stuff, and it is still valuable, but you won't notice that you are missing over 90% of the benefit.
By contrast Google has code review on every single commit before it can go into source control. Naively many people think that this would be a heavy-weight process. But it doesn't work out that way in practice. It turns out to be massively easier to review small pieces of code in isolation. When issues are found, it is much less work to change the design because you have not written a bunch of code around that design yet. The result is that it is much easier to do thorough code review, and much easier to fix issues changed.
If you wish to do code review like Google does (which I really, really recommend), there is software to help you do so. Google has released their tool integrated with Subversion as Rietveld. Go (the language) is developed with a version of Rietveld which is modified for use with Mercurial. There is a rewrite for people who use git named Gerrit. I have also seen two commercial tools recommended for this, Crucible and Review Board.
The only one I have used is Google's internal version of Rietveld, and I was very pleased with it.
A technique I've used on multiple teams is this:
- developers can integrate source on their own branch or local repo without review
- developers can integrate with the trunk/master without review
- code must be reviewed, and the review comments addressed, before it can be integrated from trunk/master onto a release candidate branch
It's the code author's responsibility to seek review, and the release branch maintainer's responsibility to ensure that only reviewed code is merged.
There are tools that support code review, but I've never used them. Tracking who did the review for any merge can be done inside the repo. I've used svn properties and perforce jobs attached to commits to show who reviewed what.
I've never separated code for review by commited/non-commited criteria - the only criteria I've encountered is that unit tests and integration tests are green.
As for tracking I would recommend to update the flow in your favorite issue tracker. For exampe instead of:
- Product owner -> Analyst -> Developer -> QA -> Release engineer
You may want to introduce one more stage (review):
- Product owner -> Analyst -> Developer -> Reviewer -> QA -> Release engineer
Therefore for every ticket in Implemented state you can assign a reviewer and only Reviewed tickets will advance to QA.
I have only the one experience of code reviews, so I can't say how good it is.
I was working with a small(~10-15) group of coders, and we were using VS Team Foundation Studio. We were asked to commit code about once a day, and before each commit code was to be reviewed by somebody else in the group (hopefully by somebody also involved in the project). During commit, the name of the person was also included in a field.
I worked on a team who code reviewed everything that was checked in on a change by change basis during a couple of reviews a week. This meant we weren't always right up to date with code reviews but it achieved what we set out to achieve.
So first, ask what you want to achieve by reviewing the code. In our case, it wasn't to catch idiot developers, there was an assumption of competence rather than an assumption of incompetence. It did allow the team to get an overview of other areas of the system, and allowed some questionable design decisions to be corrected before they became set in stone. By questionable, I mean there's always more than one way to skin a cat, and not everyone is aware there's a cat skinning knife already in the toolbox, so to speak.
The way that we tackled code reviews was that every task from our project tracking software was reviewed. At the time we were using Mantis and SVN. Our project commits were tied into both systems. Every commit had to be tied to a task in mantis. Once the task was completed a status of "Ready for Review" was assigned to it.
RFR items were then either picked up by anyone who had some free time for reviews or was assigned to a specific person for review. On Fridays all RFR items had to be reviewed before the end of the day so that there weren't carryovers to the following week.
The only problems that we ran into with this process was large items that had a ton of files. To handle this the coder and reviewer would get together and the coder would run through the changes until the reviewer understood them. They would do the code review together.
This process broke down when management dictated that if peer programming was done a separate code review was unnecessary. Developers became lax about the process and small stupid bugs began to be introduced. Eventually we went back to the original process and things came back together.
On my team we've been using a practice for the past year or so that seems to work very well.
Our organization uses Perforce for version control. Perforce (as of a year ago) includes a feature called Shelving. With shelving, I can "shelve" my changes for a particular issue. They are stored in the version control system but are not checked in. Then I ask another developer on my team to review the code.
The other developer can view my pending changes in Perforce from his own computer, and compare the changes to the most recent revisions. He can also "unshelve" on to his local machine if he wants to try out my changes. When he's completed the review, he lets me know. I then check in my code with "Reviewed by Bob" at the end of the comment.
This has worked really well for us. First of all, code reviews in general have proven to be extremely helpful. Additionally, Perforce's shelving feature allows us to do the reviews without checking in or any major difficulty even though our team is geographically widespread -- that is very important. And it works great.