Continuous Code Review
Continuous code review is reviewing all the code, all the time.
That’s the theory. In real life, some questions come up:
How do we track which code has been reviewed?
All the code?
And when is all the time?
Question 1: How do we track which code has been reviewed?
Answer 1: One cannot, and need not, mark code as reviewed.
One cannot, because, for example, if one reviews a file when it has 100 lines of code, and then later it has 120 lines of code, one may have to re-read all 120 (probably more quickly since much is not new) in order to comment.
And code is not reviewed line by line, but idea by idea.
Use a code editor that maintains a symbol tree so that the reviewer can jump from use to definition and back.
One need not mark code as reviewed because all code should be reviewed.
Question 2: All the code?
Answer 2: Yes, all the code that has been included in continuous code review.
During start-up, one might mark components or modules as having been included in the continuous code review process.
In steady state, by definition, all code is reviewed.
Question 3: When is all the time? Pair programming? Or surely, before every check-in?
Answer 3: None of the above. Try once a week.
The idea is to direct a constant flow of intelligent, objective comments to the developer, as soon after coding as possible.
Code review is not testing, and it is not certification. It is a tool to help the developer fix (improve) the code s/he has just written, to improve the code s/he is about to write, and in general, for the developer to become better at detailed design and coding.
Pair programming is not code review because the “non-driver” is not objective—s/he is part of the pair.
And it does not matter whether code is ready for check-in or not.
All you need is enough newly-written code for a reviewer to understand what the developer is trying to accomplish.
Get started!
Hi Tom,
You are right on every account. I would even say (assuming nobody is hearing) that some parts of the code (even large parts) are reviewed more than once — each time in a different context, or as part of a different interaction (with other parts). So in fact, you review 150% of the code. But I would settle for 100% as a start 😉
Lidor
Lidor
February 12, 2007 at 2:18 pm
I like this post, but I have to disagree about reviewing once per week instead of before check-in.
The problem with delaying the review is that problems that could have been discovered and fixed are compounded. New code builds on a bad decision and now the fix takes much longer to implement.
I would agree that if you do a reasonable amount of technical design before starting, then your way is best.
Jason Cohen
December 15, 2008 at 8:40 pm
The advantage of review before check-in is that you keep the codebase clean. That’s what you’re saying. The disadvantages of review before check-in are that reviewer availability can delay check-in, and that such reviews are shorter and more superficial, concentrating only on the change to be checked in.
Really, both pre-check-in and weekly are needed, and I’ve seen an organization that tries to do that. Last I checked, they were succeeding on pre-check-in reviewer availability (because the company culture supports it), and perhaps the weekly too, thanks to a manager who monitors open in-depth review requests.
If I have to choose … well, it’s hard to choose. Both are needed.
As for up-front design, of course you’re right, but it can be hard to enforce in practice. The weekly code reviews, then, are in-depth enough to find the design problems and justify why up-front design and design review should be done the next time …
Tom Harris
December 16, 2008 at 6:57 pm