The crux of the matter is that it should be as easy as possible to check in the code, and as hard as possible to ship code that wasn't reviewed. Two factors are working against each other here, if you allow the developers to check often, you will end up with a reviewer that need to read quite a bit of code. On the other hand, requiring that everything that goes into the repository will be reviewed cause either dead-locks (a developer can't continue until he's reviewed) or developers working on code without the benefit of source control. What is worse, it can get to the point where you still get a big pile of code, but now it isn't even on the repository, and you can't track how it changed. That is assuming that the reviewer won't just glance at this, see that the idention is right (about the only thing you can spot right away with reams of code) and approve it.
My propsal is (like everything this days) use rss in conjuction with the source control tags. You need to have two tags, un-reviewed & reviewed. Developers check their code as un-reviewed and then each reviewer subscribe to the code he's responsible to watch over, (either by module, by person that did the check in, phase of the moon, whatever) and then he can watch the changes. The advantage in that is that the reviewer need only to digest small ammount of code each time (hopefully, they won't neglect this, because then they would again need to face that big pile of code :-D ).
The code that the reviewer will see will be properly displayed (i.e, not diff unless they want them.) proper change viewing can be accomplished with css today, so that shouldn't be too much of a problem (or they can just use the normal tools for code they always use). You would also get the log from the source control as a heading, telling you what this is about. The advantage here is that the developers are now encouraged to write logs that explain why things are happening, rather than what is happening ("added hbm editor" is not very helpful when I gives you ~3,000 LOC, "Compiled the Hbm schema using xsd.exe and added ui with reflection" - is a little better, but what would happen is that you get stuff like: "Compiled hbm schema using xsd.exe and added ui by reflecting on ... ").
I think that this would encourage people to write more why and how than the what.
Furthermore, we can take advantage of the rss pattern usage and have a conversation about the reviewed code. For example, if the reviewer ]didn't understand / like / has a better way/etc] he could use comment this piece asking for clarifications. The developer would need to answer those concerns before the reviewer would approve the code.
When the review decides that he approves the code, he tell it to the system, and it is moved to the reviewed tag.
Advantages of this proposal:
- Encourage better documentation for checkins and for the reasoning behind the code by encouraging developer to explain in writing what they thought they were doing.
- Expose concerns and the thought process of the developer.
- Allow to explore the reasoning behind decisions and what are the possible gotchas.
("We can do that for now, but we'll need to change that to Foo when we'll get to 1.2 because of Bar")
- This put the conversation between the reviewer and the developer, which would otherwise have been lost, as a first class citizen. As important as documentation or e-mails.
- Useful for teams that works in multiply sites or has complex check-in/approval process.
- Formalize a best practice in a way that is harder to ignore.
Uses RSS, so that answers the current rage :-D
Disadvantages of this proposal:
- Add process to something that may be as simple as: "John, come and take a look at this" (But on the other hand, if it's as simple as that, it's not a formal code review.)
- Yet another place to check (Documentation, Bug Tracking System, Peer Review System, etc) before you can touch code.