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.

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?

share|improve this question

7 Answers 7

up vote 4 down vote accepted

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
Only committing once per day strikes me as a red flag. Sorry. –  btilly Feb 14 '11 at 16:07
    
Maybe. I too was somewhat surprised at first. However, it wasn't a hard and fast rule and you could "shelve" local changes as much as you wanted. –  apoorv020 Feb 14 '11 at 16:33

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.

share|improve this answer

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.

share|improve this answer
2  
+1: except "it's the code author's responsibility to seek review". Unless management demands reviews be completed, they will slide into irrelevance. There must be some kind of reward system (even casual or informal) or it won't get done. The branch maintainer answers to someone and has some kind of reward for being disciplined in checking for code reviews. This piece of the puzzle is important, also. Could you describe why folks would be disciplined at doing this? –  S.Lott Feb 14 '11 at 11:05
    
@S.Lott on the teams I've worked on, professional pride. Also, if you don't get review, your code doesn't get integrated (as described above). Therefore your code doesn't get into the product, and you've done no work that day/week/iteration. If your developers aren't motivated to do their work, you have worse problems than organising your source control repository. –  user4051 Feb 14 '11 at 11:07
    
@Graham Lee: "Professional Pride"? I scoff (but don't have much to go on.) "developers aren't motivated to do their work" is the issue. Many managers will subvert a good process by demanding a release prior to the schedule or demand additional features be wedged in. What motivating factors are in place to prevent subverting the process? What stops a manager from saying "We need this tomorrow, we don't have time for code reviews"? –  S.Lott Feb 14 '11 at 11:12
    
@S.Lott I don't know about you, but I don't release a buggy heap of shit no matter how much a manager thinks he knows better about how my job is done. –  user4051 Feb 14 '11 at 11:21
    
@Graham Lee: I try to avoid releasing buggy code. My question is "what motivates your team to avoid management from subverting your process." It's a good process, I want to know more. –  S.Lott Feb 14 '11 at 11:28

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.