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|improve this question

migrated from stackoverflow.com Aug 24 '11 at 20:32

This question came from our site for professional and enthusiast programmers.

    
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
add comment

9 Answers 9

up vote 8 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|improve this answer
    
+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
add comment

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.

share|improve this answer
6  
How does he hold off on development? He pushes to the branch which waits for review, and continues with another issue..? –  Rob Aug 25 '11 at 2:25
    
@Rob I can see an issue if one patch relies on an another but in that case you keep working and assume the dependency will be approved or don't work on it. –  omouse Jun 25 '13 at 19:49
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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.

share|improve this answer
10  
-1 for "Anything else is a waste of time." There are a number of good reasons to conduct code reviews, and pair programming doesn't address them all. –  Caleb Aug 24 '11 at 20:59
2  
I agree that "Anything else is a waste of time" probably is false, but pair programming is by any meassure the best (at least from a quality point of view) code review processes there is since it is a continous feedback mechanism while the code is being written. –  hlovdal Jan 18 '12 at 6:58
add comment

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.