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.

We are trying to switch to a continuous integration environment but are not sure when to do code reviews. From what I've read of continuous integration, we should be attempting to check in code as often as multiple times a day. I assume, this even means for features that are not yet complete.

So the question is, when do we do the code reviews?

We can't do it before we check in the code, because that would slow down the process where we will not be able to do daily checkins, let alone multiple checkins per day.

Also, if the code we are checking in merely compiles but is not feature complete, doing a code review then is pointless, as most code reviews are best done as the feature is finalized. Does this mean we should do code reviews when a feature is completed, but that unreviewed code will get into the repository?

share|improve this question
    
When it comes to checkins/pushes, most places have one major rule: Don't break the build! I.e. don't check in something that won't build. Other than that most places I been to want small and confined checkins, but never said anything about the amount. –  Joachim Pileborg Nov 8 '11 at 3:01
    
but when does the code review happen, before you checkin, or when your done with the feature? Does that mean you have code that hasn't been reviewed checked in, and that you fix any issues found by the review after? –  SpecialEd Nov 8 '11 at 3:22
    
It varies, but most places want to do code review on private branches before being merged into one of the main branches, –  Joachim Pileborg Nov 8 '11 at 3:28
add comment

migrated from stackoverflow.com Nov 25 '11 at 6:32

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

10 Answers

IMO, you should review code before it gets published to mainline so that mainline only ever has the highest quality code.

OTOH, a good point could be made that 'why bother reviewing if the CI test automation hasn't run on it?', so perhaps the best would be to give developers each a private branch that the CI server will build and test for them. That way they first commit and push there, then once it passes get it reviewed, then merge to mainline (where it will get another run through the CI server).

You should definitely review non feature-complete code to make sure that scaffolding for future features is in place, or at least that there's nothing being put in that would prevent said future features from being implemented.

Also, note that code reviews don't have to be slow, or synchronous - a tool like gerrit or reviewboard or the like can make them asynchronous and fairly painless.

(Full disclosure: I work for SmartBear, makers of Code Collaborator, a code review tool)

share|improve this answer
    
So, somebody that's working in a non-distributed VCS would have to email their changed files to get their code reviewed. Why not just check it into a branch? –  Jordan May 25 '12 at 18:33
3  
Codereview-by-email is poor practice (though better than nothing, admittedly) because it's hard to tell when the review is 'done'. Get a real code-review tool like gerrit or reviewboard and use it and stop emailing patches around :) –  pjz May 25 '12 at 21:30
    
Yeah I agree with you. You said to review code before it's checked in, which I think is a terrible idea. Check the code into a private branch or fork, and then review it. The tools you listed will support that, and it prevents code from becoming stale or people having code that is not in source control. –  Jordan May 26 '12 at 2:02
    
I'm used to using our commercial tool, which will upload the diffs in your working copy for review before you check them in - this is not unusual with non-DVCS systems like svn and CVS where branching is more painful. The process is: write code, submit it for review, once it's passed, check it in. In the meantime if you want to get any other work done you check out a new working copy in another dir so as not to lose your in-progress work. DVCS's are sooo much nicer :) –  pjz May 26 '12 at 2:33
    
Still, I don't think that's an ideal process, regardless of DVCS or not. One of the necessities of code review is not just to look at the code, but to actually run the code or automatically test the code and see what happens. You can't do that with just a series of diffs. –  Jordan May 26 '12 at 2:41
show 3 more comments

I don't know if it is the best way to do it... but I'll explain how we do it. One or more developers work on a given branch and commit their code as often as they can in order to avoid wasting time on merge that would not have happened otherwise. Only when the code is ready is it committed into the head. Now that's for the commits and branch/head thing.

As for the code review, we use Sonar as our continuous integration tool (and Maven/Jenkins for interacting with Sonar) for providing us with fresh test results, code coverage, and automatic code review every morning (builds are done nightly) so that we developers can spend a maximum of one hour every morning to fix their problems/code smells. Each developer takes responsibility (being proud too!) for the feature he is writing. Now, that's automatic code review, which is great to find potential technical/architectural problems, but what is more important is to test if these new implemented features are doing what the the business wants them to do, properly.

And for that, there are two things: integration tests and peer code review. Integration tests help in being reasonable confident that the new code does not break the existing code. As for the peer code review, we are doing it on friday afternoons, which is a bit more relaxed time to do that :-) Each developer is assigned to a branch he does not work on, takes some time to read the requirements of the new feature first, and then checks what has been done. His most important job is to make sure that the new code works as expected given the requirements, does not break our own "rules" (use this object for that, and not that one), is easy to read, and that it allows for easy extension.

So we have two code reviews, one automatic and one "human" and we try to avoid committing unreviewed code into the HEAD branch. Now... It does happen sometimes for various reasons, we're far from perfect, but we try to keep a fair balance between quality and cost (time!)

@pjz provides a good answer too, and he mentions code review tools. I'never used any, so I can't say anything about that... although I have been tempted in the past to work with Crucible since we're already using JIRA.

share|improve this answer
    
Interesting idea that reviews should be scheduled for a particular time/day... –  William Payne May 30 '12 at 15:42
    
@WilliamPayne thank you. Another thing that works for us is to schedule code review meetings, so it's clearly visible on the calendar that we are "busy". It helps in warning people that although we're here ... we're in fact not :-) –  Jalayn May 30 '12 at 19:58
add comment

Set up pair programming?

All of the code is reviewed as it is being typed without extending the process or introducing another step.

share|improve this answer
add comment

I think the main concept that will help is that of a "Staging" area.

Yes, you don't want to check in code that's broken. But you also should be checking in code frequently. Does this imply perfection? ;) No. Just use multiple areas and a DVCS like git.
This way you make changes (locally) and commit them frequently as you test and develop until the tests pass. Then you push to a staging area for code review.

You should then be pushing from Staging to other QA efforts such as browser tests and user tests. Finally you may go to a volume testing area, then finally production.

There are also workflows within this such as everyone working on the main branch or using individual branches for all efforts.

Continuous integration itself can also happen at multiple levels. It can be local to a developers machine 'until the tests pass' and it can also be in staging and qa areas for when code goes to them.

share|improve this answer
add comment

Decouple code review and continuous integration!

Why you combined them?

share|improve this answer
add comment

I really-really-really think you would need a DVCS (e.g. mercurial, git) to do this naturally. With a CVCS you would need a branch and hope to whatever god you have there is no merging hell.

If you use a DVCS you can tier the integration process so that code already makes it reviewed before it arrives to the CI server. If you don't have a DVCS, well, the code will arrive your CI server before reviewed unless code-reviewers review the code on each developers' machine before they submit their changes.

A first way to do it, specially if you don't have repository management software that can help publish personal repositories (e.g. bitbucket, github, rhodecode), is to have hierarchical integration roles. In the following diagrams, you can have lieutenants review developers' work, and have the dictator as the main integrator reviewing how lieutenants merged the work.

enter image description here

Another way to do it if you have repository management software, is to use a workflow like the following:

enter image description here

Repository management software typically helps emit notifications when there is activity in repositories (e.g. email, rss) as well as allowing pull-requests. Code-review can organically happen during pull requests, as pull requests typically make people engage in conversations to have the code integrated. Take this public pull-request as example. The integration manager can actually not allow code to arrive to the blessed repository (aka "central repository") if the code needs to be corrected.

Most importantly, with a DVCS you can still support a centralized workflow, you don't need to have another fancy-awesome workflow if you don't want to... but with a DVCS you can separate a central development repository from the CI server and give someone the authority to push the changes from the dev repo to the CI repo once a code-review session has been done.

P.S.: Credit for the images go to git-scm.com

share|improve this answer
1  
The folks at github use pull requests to do code reviews and it seems to work well according to Scott Chacon, Zach Holman and others. –  Spoike May 30 '12 at 6:55
add comment

Why not have more than one repository? One for "daily" work, driving a continuous integration server, running all the unit tests & integration tests to get the nice tight feedback loop, and another for "stable" work, where commits are less frequent, but have to go through review.

Depending on the path that changes take as they move through the system, this can end up being a bit of a complex solution, and might work best when using tools like Git, or Mercurial Queues, (caveat: I have used neither in anger) but lots of organizations do something similar.

share|improve this answer
add comment

We use git flow for our repositories, and we do our code reviews when it comes to merging into the develop branch.

Anything in develop is complete, deployable and code reviewed.

We also have CI set up for our develop and master branches.

share|improve this answer
add comment

Does this mean we should do code reviews when a feature is completed, but that unreviewed code will get into the repository?

Well above is the way I've seen it done in at least three projects that were intensively using continuous integration and per my recollection it worked like a charm. This practice is known as post-commit code reviews - search web for this term if you're interested in details.

  • On the other hand, the only case when I've been in project trying to "marry" pre-commit code reviews with CI turned out rather painful. Well when things went 100% smoothly, it was OK - but even infrequent interruptions (like when main and backup reviewers were both unavailable for say a few hours) made noticeable stress. I also noticed that team morale somewhat suffered - there were a bit too much conflicts.
share|improve this answer
add comment

First, we should clarify the concept of "continuous integration". In traditional developing methods, continuous integration means we can integrate and build our source code repository everyday, which will avoid the pitfalls of "integration hell". The code reviews are always between the period of Coding and Unit testing. We must guarantee the code merging to the branch can compile with no errors. There seldom has the situation that parts of the feature merges to the branch because it is difficult to handle the coherence of interface and compile errors.

Continuous Integration is popular in the Extreme Programming process. Test-driven development adds Pair programming, which is actual part of a code review process, making continuous integration easy to implement. Extreme Programming itself is a continuous code reviewing and integration process. The code reviews exist in everywhere.

In some open source communities, the code reviews are executed just before the code merging to the branch. It is always the most experienced people in this team to do the code reviews and decide whether the code can merge to the main branch. In this way, the period of Continuous Integration is a little bit longer but the code quality is a little bit better.

Return to the question. There has not standard answer for when to do code reviews, and it depends on your original development process and the actual implementation of your continuous integration.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.