Code Review vs. Check In often?

Question

Following Jeff's article about checking in often, some commenters couldn't resist spewing "Welcome to obvious land!" kind of comments. That's understandable, in most situation it's obvious that checking in as soon as something is finished before proceeding to the next feature is the way to go.

But what about places with strongly enforced mandatory code review politics? Asking a coworker for a review every 30 minutes is quite a productivity killer.

I would be interested on a solution, or at least a possible compromise that would let us do both without crippling team productivity.

Answer 1

How about if you pair program? Then the theory is that won't need a peer review. The pair-programming is the Peer Review

Answer 2

I don't know that there is a solution to your problem. To me, it sounds like your situation falls into the agile-run-amok camp. It sounds like your productivity is already crippled if you can't even check in (for example) a null check to fix a crash without someone looking at it for you.

Answer 3

Is it no code at all, or no code in the trunk without review? I think using branching (hopefully you are not using VSS!) would possibly work: i.e. only trunk changes need peer review.

Answer 4

That is silly. Even if you require code review does not mean it can't be checked in. That's what source code control is for! Especially if review is bottlenecked, batching it is the only way to go.

Answer 5

Depends which branching strategy you use. If your repo is working on release branching, the trunk is meant to be unstable, but maybe your policy stems from some really bad experience in the past. If you are doing feature branching, then you can have a rule that says no merge back to trunk without peer review. Doing diffs at that point should be quite efficient to review with a colleague and should remove the bottleneck.

But changing your branching strategy is no small thing. Good question...

Answer 6

In most cases what is in source control isn't what is going to be imminently released to the customer. Therefore I would perform the reviewS after the full feature has been checked in, allowing you to edit and/or rollback as necessary later on.

Answer 7

Not every source control system facilitates this, but I've used this process when working on a large project:

• Every developer gets their own stream.
• Developers deliver completed features from their stream to a parent integration stream after code review.
• Developers update their stream from the integration stream frequently.

The use of developer streams means that developers can stay visible and have backups of their work in the system. It is relatively easy to collaborate with someone else - they can just check out your stream. By creating a new developer branch per feature/defect, you can isolate changes fairly easily - say you need to stop working on a new feature to fix an urgent bug.

Answer 8

My company follows somewhat disparate processes between divisions. One of them (that I recently left) has adopted pair programming and the check in comments note that the code was "pair reviewed". The product that's being worked on, however, is somewhat mature and the development process can tolerate the slowed productivity.

The division I currently work in follows the check-in early, check-in often model. We are developing a new product with relatively few engineers. Each of us monitors the check-in's through email notifications and reviews the interesting parts on an ad-hoc basis. My current team is pretty well seasoned, though, and I think we can get away without formal reviews due to the quality of the engineers working on the project.

To answer your question, as the previous poster suggested, pair programming is probably the best compromise. Personally, I'd rather get a good team together and let them check-in code as quickly as possible. (In my opinion, the "no code in the repository without review" is an attempt by management to increase quality without hiring quality engineers. Please take that with a grain of salt, though; I'm a bit jaded by my current employer.)

Answer 9

I prefer to keep a stable trunk and branch for releases, merging back in to the trunk after production. Labels are used to indicate code-readiness and COULD be paired with a code review strategy. Releases to SIT/UAT get their own special label.

On the team I'm currently a part of there are only 3 of us writing so peer reviews are constant and brutally entertaining, educational, & honest. I'm not one who gets emotionally attached to code I've written so I love a good review. I've learned a lot in a short period of time from them. That being said, they're not required; they sort of organically happen.

If peer reviews are required there's no reason to suspend check-ins. Simply make a label that indicates the review has taken place and make it the reviewer's responsibility to apply that label to code once it's passed review.

Answer 10

We do code reviews on a story level (agile shop here) so once you are done a task related to a story you do a code review and then check in.

Answer 11

Some revision control systems make this easy (those with a decentralized strategy). I've experimented with using several as local middle-layers to a company subversion server.

I've used svk to maintain a local subversion repository I can commit to often, then push changes (individually or as a single combined commit) to the parent repository.

svk mirror
svk pull
svk push

This works pretty well, but development on svk seems to be sporadic.

I've started to try the same thing using git. I've made a local clone ok, but have yet to be brave enough to attempt to push local commits back up. I'm sure it's possible though.

git svn clone
git svn fetch
git svn dcommit

With this private, local repository, you can commit all you like, then bundle or cherry-pick changes to push to the master repository. (Or create a patch that can be peer reviewed.)

Best of both worlds perhaps? These alternative revision control tools aren't likely to gain traction at work, but they're really useful for local commits or to be able to commit when the work repository is inaccessible (I'm offline, travelling, etc.) And they still sync with the parent repository.

Answer 12

There are a lot of good answers. Most tend to propose a feature branch for each developer that require a review to merge into the trunk. However, this reduces source control to a backup technology rather than something that makes codes between several programmers be easily shared almost as soon as it is written.

As flubba mentioned, the best solution would probably require to change branching strategy to the most appropriate for the team, but I'm affraid the inertia in our company is so strong this will require me to spend a lot of influence points, probably without success.

I will do my best.

Answer 13

@Coincoin: I don't agree that individual branches reduces source control to a backup technology. I do think that backups are one of the reasons to use source control, but there is more value from source control in an individual branch:

• I can make small steps, documenting them carefully as I go, even if they're not ready for prime time.
• Understanding how we got here is possible, especially when the steps are very small.
• Looking at the changes from the shared branch, you can see the big steps, then drill down to the individual branch for the small steps that got there.

Still, the rule of "if it's not committed, it doesn't exist" still holds, and is more pervasive: "if it's not committed/merged to the shared branch, it doesn't exist".

I do agree with those who say that checking in often is a great practice, and that pair programming is the solution to eliminating the latency of code reviews.