peer review code before or after check in

Question

Do you peer review code before or after checking in to the repository?

What do you see as the pros and cons of each option?

Answer 1

I can't think of a comparable pro/con argument for this: enforced code reviews before check-in sound like a really, really bad idea on the onset.

It would be impractical to enforce a peer code review before check in, and it effectively becomes nothing more than red tape.

Besides, this also discourages people to check-in (commit) early, check-in often, which instead increases the amount of errors in code instead of decreasing it.

Answer 2

Peer review afterwards, then the reviewer can easily diff etc. from their own box. Email alerts from the source control system should be setup for those who are considered necessary to review ;)

Answer 3

PVCS is the only source control I've used that handled this nicely. Code was checked in at the lowest level every day, and only a check-out by the developer that checked it in would get their changes. When they finished they promoted their changes to ready to review, and let someone know to review it. At this point a fresh check-out by a developer would grab their changes. Once the review was done it was promoted to ready to QA, and the nightly build would get the changes, then QA would promote it once they were finished and it would end up in the finished product.

Answer 4

We have a policy of only checking in code that's tested and working. There are advantages and disadvantages of this policy, but that's not what's being discussed here. Given this policy, review before check-in is obviously the right way.

I actually find it much easier to review before check-in anyway; just change to the right directory and cvs diff, or better still use something like emacs's cvs status command to see what files have changed and ediff them. With a better source control system review after check in might work just as well but CVS makes it hard to see what was checked in together so it can be reviewed together.

I'd prefer to use a DVCS, and then review obviously should happen after checking in to the developer's private branch, but before merging into the main tree.

Answer 5

A lot of this depends on what branching strategy you are using.

You want your stable branch to ALWAYS compile clean. That said, you may have an unstable development branch that is used for early integration with other developers, and to ensure that code is actually backed up properly - you want that one to be checked into frequently.

Then when a task is ready for review, it is already in the dev branch, and the reviewer then pushes it up to the stable branch (easier said than done, but that's a whole other discussion).

The major downside of pre-checkin code review is that it puts a major bottle neck on getting changes into the system for other developers to integrate with early enough.

Answer 6

I second Jon Limjap answer. Unless your Version Control System has a "private branch" mechanism (local commit), I would recommend setting-up peer code review after check-in.

And I would definitively not go with the "one branch per developer" way. That lead to way to many merges, since each developer, before committing his/her code, should first merge evolutions from the common branch into his/her dev branch, fix any last compilation error, make the review and then commit back in the common branch.

If several developers are involved in one development effort, they should do it on a same common branch, and commit often (as long as the build is not broken), while setting up a peer code review on a regular basis (but not always just after all checkins).

A simple request can then display all files modified by a developer, starting from the last review: that list can be used by the reviewer on the next code-review, again avoiding a systematic review after any check-in.

By separating checkins and review occurrences, you allow checkins to be made at their own optimal pace.

Answer 7

I will respectfully disagree with Jon's answer. The value of checking in early and often is related to several things: the quality of the code being checked in, the volume of that code, and the degree of overlap between developers.

If you generate quite a bit of code, it's important to do something to support code quality prior to making it available to other developers. The act of checking in code itself doesn't remove or prevent defects. If you are using CI tools, they'll certainly let you know if you've broken the build, but it's not difficult to introduce defects that don't break the build. Inspections can be more effective than system, integration, or even component testing in terms of removing defects from code, and in a high-volume environment, or one where several developers are not co-located, you may choose to spend more time on inspections and less time on coding in order to keep the quality of repository code high.

However, pretty much every SQA activity is like a "one-size-fits-all" hat: it doesn't fit "all". Many companies will have a development process that allows them to produce quality code without using peer reviews prior to committing code, and there's nothing wrong with that. As pointed out in the related question, pair programming can be an effective way to avoid roadblocks due to required inspections. (I suppose you could argue that you're not actually avoiding peer review, you're simply doing it while you're coding.)

If there is a question about what you should do at your own place of work, I'd recommend collecting data on your current process. If you can demonstrate that you produce quality code using your current development process, then I think that's really all you need to know. If you're seeing too many defects in released code, then maybe it's worth changing your approach.

Answer 8

I think you should use DVCS and then problem is kinda not existing. You commit (to the local branch). Send diff to review, and the reviewer can push the code by himself or let you do it...