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.

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.

share

migration rejected from stackoverflow.com Sep 9 at 20:29

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.

closed as primarily opinion-based by Mike Brown, MichaelT, GlenH7, Kilian Foth, ratchet freak Sep 9 at 20:29

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? –  blueberryfields Aug 24 '11 at 20:50
    
@blueberryfiels good question, I d ont really know what is the best option... –  JohnJohnGa Aug 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. –  Chad Sep 8 at 20:46

7 Answers 7

up vote 9 down vote accepted

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.

share
    
+1 for the extension –  JohnJohnGa Aug 25 '11 at 5:38
    
Using Review Board with git here, works great. –  Bjarke Freund-Hansen Aug 25 '11 at 7:27
    
Accepted, review board seems to be a great solution, I will try it! –  JohnJohnGa Aug 26 '11 at 7:11

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 !".

share

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.

share

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.

share

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

share

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.

share

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.

share

Not the answer you're looking for? Browse other questions tagged or ask your own question.