Comment If you're asking, then you're doing it wrong. (Score 1) 345
There's two aspects to make it work:
- Tools. You should put some effort into your tools. The features to aspire to are:
- tight VCS integration
- email integration
- visual diff viewer
- inline comment annotations
- web interface is preferable
- Policy.
- Every change should be individually reviewed, before it is committed (reviewer(s) give the final OK).
- Smaller changes should be encouraged. The smaller the better, but more than 1000 lines is too big. Reviewers should request large changes to be broken up if necessary.
- A single reviewer per change is adequate, on average. 2-3 is also common.
- Reviews should also be cc'd to team and other stakeholders; unsolicited reviews/comments should be encouraged.
- A consistent code style should be enforced in reviews.
The attitude of the engineers is important, but difficult to enact. It's particularly hard when you're bootstrapping a new policy like this into your engineering culture. Although the reviewer has the final say, in reality they don't have any real power over their colleagues' actions. So they should focus on constructive, concrete issues and back off once a reasonable debate has occurred. Design reviews are important because if design issues come up in code reviews it can get ugly.