What are the most efficient code review processes?

Question

I am looking for an efficient code review process where nothing can be pushed without being reviewed by another dev.

For instance (we are using Mercurial):

• push to another branch and the reviewer will merge with the 'head'

• push directly to the reviewer

The main issue here is that everybody can push to the default branch, so it doesn't enforce the code review.

Answer 1

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.

Answer 2

That does not sound efficient in the least, you are forcing a developer to hold off on development as he waits for someone else to review it. Worse you are interrupting a productive developer on what could be an hourly basis. My recommendation is to use automated tools, such as FindBugs and PMD, this will allow each developer to fix as many static issues that can be found. I would recommend a weekly code review session over all critical pieces of the application that were checked in. Now if this is a system that handles the power system at a nuclear power plant ignore everything I just said and force a code review before any commit to the head revision.

Answer 3

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

Answer 4

Gerret and Git offer a light weight system. We are rolling Gerret out now (Used git for a while) and very pleased with the workflow, and sounds much like what you are looking for.

With Gerret the change is pushed to Gerrit. It cannot be pushed to the main branch until it has been reviewed.

Unfortunately for you it's a GIT based tool, I am not familiar with Mecurial alternates, however if the review issue is big enough moving to GIT/Gerret could be worth while.

Answer 5

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.

Answer 6

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.

Answer 7

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.

Answer 8

One way to achieve your goal is to set a "one branch by feature policy". So, every development is performed on a branch, and the merge is applied to the master/trunk only when the task is DONE.

It's up to your team to define the DONE, and it can include code review.

Of course, you'll have to face the usual "I'm affraid of branches" or "I don't want branches, branches suck !".

Answer 9

The most efficient code review process is pair coding. Pair a senior dev with a junior and the review (and knowledge sharing) happens as the code happens. Anything else is a waste of time. (Even remote devs can pair code with screensharing tools and skype).

I stand by what I said. What can possibly be done in a code review that cant be done with pair coding (plus automatic code analysis). Ensuring coding standards are met? Knowledge transfer? Just like post hoc unit testing is less efficient than TDD, post hoc code review is less efficient than pair coding.

let's say i spend 8 hours writing a feature. It requires a refactoring of several classes. I then make a shelveset and document my changes and submit it for a code review. The reviewer looks over my code and suggests changes. I then implement and resubmit. In more formal review practices there may even be a meeting with several members of a review board. Do you see where I'm going?

Instead of all the overhead and waste involved in a post hoc review. Have the senior members pair with juniors. If a senior member has a concern, he confers with other senior members to come to an agreement for addressing the issue.

The junior developer learns quicker. The architecture and coding styles are honored, and the feature gets delivered quicker. And to top it all time isn't wasted poring over code that's already been written.