This question came from our site for professional and enthusiast programmers. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.
If this question can be reworded to fit the rules in the help center, please edit the question.

Does it matter to you which of the developers are doing the review, or do you just want to guarantee two eyes on any piece of code?
–
blueberryfieldsAug 24 '11 at 20:50

@blueberryfiels good question, I d ont really know what is the best option...
–
JohnJohnGaAug 25 '11 at 5:43

For the bounty I am looking for more than just opinions. I am hoping to see something that spells out best practices for code reviews.
–
ChadSep 8 '14 at 20:46

7 Answers
7

You should try out Review Board. My company uses it with Perforce and it's very efficient and doesn't interrupt the normal workflow (well, anymore than Perforce does). I believe it works with Mercurial. You can also look at the Mercurial Review Board extension.

Buddy systems work well, when you know that the pair agrees on silly style things (no holy wars over fill-in-the-blank). All reviews from person A go to person B, and all of B goes to A. In the event of a sick day or vacation, the lead becomes your buddy. The reciprocating setup is important. Strangely it doesn't work as well with a 3-person triangle arrangement (A sends to B to review, B sends to C, C sends to A). Perhaps it is that there is only one bidirectional communication channel instead of three one-way affairs. Once your buddy is happy, then you check your code in (so everyone has check-in rights).

Our team has a policy that all nontrivial (fixing a typo) changes are peer reviewed. We all work in the same room, when someone needs a review they ask the group at large. We all operate on the honor system that we get a review before committing.

The only alternative is to designate a few trusted committers, and give only them write access to the repository. Anyone wanting to push needs to get a review, verified by a committer, and then submit a patch for the committer to push. Git supports this innately (it has the concept of author, which is not necessarily the committer), I'm not sure about Mercurial.

I am unaware of any automated system that requires reviews, that can not be bypassed by the humans who will go through whatever motions are necessary to push without actually looking at the changes.

My current company has a pretty decent code review process IMO. We're using MKS Integrity as source control and we've implemented some (as far as I know) custom code to have MKS manage code review as part of our SDLC. Also let me state that we have over 100 developers, so this may be a but much for a smaller shop.

We have 3 set of reviews: application code, UI code and database code. Therefore, we have three groups of reviewers. Application / UI code is kept in a separate branch from DB code, so it's easy for us to lock down which group needs to see which code.

We have global code and customer customized code. All code goes through your Team Lead first. If the code is not custom to a client, it then goes to the Global Review Team and they validate it for use on a global scale. We check for syntax specified in our Coding Standards, then we check for anything that just looks wonky or that might cause instability in the overall application. (We're currently altering our SDLC so that we can discover most of the latter prior to development.)

Once code passes whichever reviews were required, the developer is then able to merge the code from their development branch to the integration branch. Our QA and release people take it from there.

One way to approach this is to integrate your issue tracking system with your source code management. Most issue tracking systems and SCM systems have some provision for user-defined scripts ("hooks") that run when specific events occur. The idea is to require that each set of related changes be done in a separate branch, and that each branch be associated with at least one issue in the issue tracker. You can use hooks in the issue tracker to restrict certain issue state transitions. For example, if your issues have these (among other) states:

Open: issue in active development

Resolved: development finished, ready for review

Approved: code reviewed and approved for merge

you can write a hook that requires that the person from Resolved to Approved must be someone other than the person who transitioned from Open to Resolved. You can also write a hook for your SCM that requires that the person merging the code must be the person who transitioned to the Approved state for all the issues associated with that branch.

I have to confess that I'm not a Mercurial user and so only know what I've read about it. I know it does have a hook mechanism, and I think the recommended setup is to have a repository set up on a server. If that's right, then you'd probably only need/want to install the enforcement hooks on the server repository; after all, the idea isn't to prevent users from checking their code in, it's to prevent them from merging back to the main branch.

I've worked in several places where we've had something like what's described above, and one thing I can tell you is that a small number of restrictions generally works better than lots of restrictions. The more you trust developers to do the right thing, the easier it is for them to actually do that and get their work done. In fact, I'd say that your first step should not be to add a bunch of hooks to your issue tracker, but to say to your developers: "we'd like every single line of code to be peer reviewed before being merged, so please don't merge without first getting someone else to check your work." If that doesn't work, then add a few hooks to start enforcing the process. That may or may not be practical, depending on what you're developing and who you're working with, but the point is that less is often more when it comes to enforcement mechanisms.