When to review code? Before or after checkin to MAIN?

Question

Hi there,

Browsing stackoverflow, I noticed a number of posts stating that some developers advocate code review before checkin to MAIN. However, can this really be enforced? And if so, surely it reduces the likelihood of code being refactored due to the increased overhead involved?

Personally, I prefer the approach of employing continuous integration to catch anyone who breaks the build, and then perform code reviews on a more coarse-grained basis at the end of a significant piece of work. I don't mandate code reviews for minor refactorings.

I'm interested to hear your opinion.

Answer 1

In my opinion, you should have a code review every time you check in. The cost of fixing things goes up quite a bit when it's in the tree.

Answer 2

This probably depends on the type of review. If you're talking about a formal inspection which might take hours/days of planning and setup, I'd argue that checkin is fine. If your shop employs more informal reviews/walkthoughs, make those as painless as possible and enforce that all code is reviewed before checkin.

If your reviews involve "increased overhead" then you're right, you might not want to enforce a review before checking. If your review is a more informal affair, then I would advocate enforcement of a review-before-checkin policy.

Answer 3

Another thing to consider is pair programming. This provides you with even more immediate feedback, you review code as you write it. This is one of its biggest pros.

Of course, if you don't want to do this, I'd suggest against a review before checkin policy. If your guys are that bad that you feel you must, some remedial training may be in order.

I tend to review occasionally in my downtime and when I'm going back over code to fix/change things.

Answer 4

I think the best approach to get rid of the code review overhead before each checkin is to make devs work pair-programming. Beside this you can designate a team member to be the official reviewer for an X specific project module. And this guy can make more deep reviews daily.

Answer 5

We hold a weekly lunch meeting to review code that we written the week before. We generally try to make sure to review code changed as part of an iteration before that iteration is complete. That way the developer has a few days to make the recommended changes before the final functionality is delivered.

We've found that because we review the guidelines on a regular basis (once a week), generally the changes that need to be made are small and can be implemented fairly quickly.

ADDITION: We use FxCop to check the code with every build automatically. So our reviews are more at the soft level around approach, framework usage, structure, etc.

Answer 6

Every time you complete a feature (make sure to schedule time for it as one of the feature's tasks!).

Code review Every check-in is very bad - I want to check in every hour or even more, it's impossible with constant code reviews. Furthermore, it's unnecessary - your main concern when checking-in is to make sure you don't break any existing code, and 90% of the time you don't need a CR for that.

When working on existing code that you're not fluent in, you might call people over for a quick 3-minute review of the delicate pieces before checking in - this is especially important in projects with less-than-optimal code coverage by tests (in such a project, even if all unit/integration tests pass, you still don't know if your code is good).

Answer 7

If you set up the version control to send diffs to all developers, there would be a chance they do the "minor reviews" semiautomatically.

Personally, I prefer the approach of employing continuous integration to catch anyone who breaks the build, and then perform code reviews on a more coarse-grained basis at the end of a significant piece of work. I don't mandate code reviews for minor refactorings.

My feelings are the same. To me, code review is a quality check and learning experience about some unit of code, not the gradual changes. The learning from others part is imho as important as the other benefits of code review; any review I've done I've learned from.

Answer 8

Pair programming is not always feasible nor desirable; people need time to think and work uninterrupted. Further, reviewing every commit instead of larger changesets leads to ginormous commits that can be hard to deal with and understand (fewer commits == fewer reviews == developers spend less time in meetings).

You want developers committing often. It is not unreasonable to insist commits not break the build, but you want many small logical progressions of commits as a feature developers. A distributed version control system can help here.

On a small team with clear seniority, I've found spending 30 minutes a day reading diffs from source control to be fairly effective. It's described here in my blog.

Otherwise, you will need some tool support (e.g. Crucible). Scheduling meetings and going over things on the projector is too painful; you will stop doing it.

Answer 9

I vote for before. Failing faster = failing cheaper.

The problem with waiting until you break things to fix them is that it only works for small teams. If you have big teams then the likelihood of someone breaking something on any given day goes way up (consider a team of 100, all it takes is a build breaking defect rate of 0.5 per dev per year for the build to be broken, on average, EVERY WEEK).

Also, in terms of maintaining high code quality, the best time to improve code quality is when it's most malleable, which is before it's checked in.

Answer 10

It depends on what kind of source control policy and software policy you have. If a developer can check in code that they want to preserve but isn't the final product then you have a different situation than a place where every check in results in a build that is stamped and possibly shipped to production so that the source control is very tightly locked down. Are there multiple check outs where something like Subversion or GIT compared to something like Sourcesafe that can prevent multiple check-outs.

Another point in this is what is meant by a review: Is this someone going over another's code, or is it someone showing the rest of the team, "This is how I implemented feature ABC," or just the leads? This varies considerably and is somewhat tied to the source control point in terms of what kind of work environment do you have in terms of how are things organized and structured.

Answer 11

Code reviews aren't a very good way to prevent build breaks. A single-step build process that's the same across all enlistments (dev and build machines alike) should prevent most build breaks. Regular testing (both automated and manual) is a better way to catch most bugs.

Code reviews are useful because:

1) They guarantee every line of code has been seen by at least two pairs of eyes in case the first pair goes on vacation or leaves the team.

2) They facilitate an exchange of ideas between the code reviewer and code reviewee.

3) They provide an extra level of accountability.

Finding bugs during a code review (let alone build breaks) is a nice bonus, but not all that common in my experience.

The advantage of doing code reviews before checking in is that you don't propagate your code defects (or build breaks) to the rest of the team. It's much cheaper (for the team as a whole) to detect as many problems before checkin as possible. The larger the team, the more pronounced this effect.

Answer 12

Code reviews on every checkin will reduce the checkin count and resize the checkin volume. Such large checkin will increment the risk of a break.

A code review can not prevent a break. If one people think the code is ok then another can also think it very fast. A continuous integration with a high code coverage is better. Writing a test case for the most checkins make more sence.

Answer 13

Many of the code review policies mentioned already are good practices, in the right context.

• For a major feature, have a branch you are working in for all feature checkins. When the feature is ready to be brought into main, review the combined changes. Feature branches should take regular merges from main so the branch doesn't diverge too much and copmplicate the review.
• For checkins going directly into main, a code review per checkin is generally appropriate. Each build break in main has large impact on all the developers working from main.
• During the final week(s) before the product ships, when the bar for defect acceptance is high, not only should all changes be reviewed, but they should be reviewed by only appointed senior developers so as to lower the change risk even more.

At my current company, we occasionaly allow post-checkin reviews if (a) the submitter is a senior dev and (b) waiting for a review would substantialy impact others. For example, someone makes a fix at 10pm. Waiting until the morning before doing the checkin can impact other developers, as well as automated tests and QA itself. We enter a defect/task item to another dev to have them do the review. That way the review does not get forgotten.

Answer 14

Get code checked in as soon and as frequently as possible. The sooner the code review is done to the time of the code change the better. Don't let quality control and process stall productivity. Code review was made for the developer, not the developer for the code review...

Answer 15

It all depends how your organization is set up.

For small teams the strategy that I would advocate is that all checkins should be directly to your trunk, diffs should be sent to the team, and that all checkins should be reviewed by the team from those diffs. This has little overhead, and encourages lots of small checkins (that are easier to review by email). Depending on your group, it likely will sense to rotate who has the primary responsibility for reviewing code at any given time. You then branch testing releases off of the trunk, send them through QA, then release to production after QA has approved them.

Larger teams probably do development in multiple branches. Depending on how you are organized you have opportunities for code review before check-in (works with pair programming), before merge (this is effectively how many open source projects work), or at regularly scheduled points in your development cycle (these would be more formal reviews).

The key difference here is that small teams are all about reducing process, while large ones are all about controlling communication overhead. But don't dismiss the small team dynamics as unprofessionalism. Small teams enjoy huge productivity advantages over large ones. Lawrence Putnam did some fascinating research on this which you can find quoted in Software Estimation: Demystifying the Black Art by Steve McConnell (see page 229). He found that for medium sized projects (~50,000 lines of code) the average 5-7 person team will complete in less calendar time than an average team of size 15-20. I would bet that the small team has more coherent code, so they probably accomplished more.

Remember that these are average teams. Note that locating 5 exceptional people is easier than locating 20 of them. It is therefore easier to create exceptional small teams than exceptional large ones. Given documented individual productivity differences, I would bet serious money that an exceptional team of 5 people will easily have higher throughput than an average team of 100 people. And their turnaround time on small projects will be much faster. Even if you pay them four times as much per person, that's still 1/5 of the cost!