What are the good code review/walkthrough techniques?

Question

Our team is pretty committed to code walkthroughs. I define a walkthrough as: Where two people sit down next to a computer and look at a diff prior to committing.

I do find them to be very time consuming, but it is often the case that good results are generated from such a procedure. So my question is, what is the single technique/techniques that I can do to make a walkthrough as effective as possible, both as the reviewer and as the presenter?

Answer 1

If they're going to sit side-by-side anyway, why not just pair-program on it? Your results will be better because a truly good code review requires far more time than people are typically willing to give. Otherwise all you typically get are the superficial things (e.g. you don't need those namespaces, your curly brace is in the wrong place, etc.). It is very hard in an a posteriori code review to truly find the interesting design and implementation flaws.

Answer 2

You know, my personal take on this is that code reviews/walkthroughs are part of an overall approach to software development. I'm not too militant about my stances, but I would agree at least in part with Peter that you may only find the nits, unless you are reviewing the code written by someone who simply hasn't solved the problem and is coming to the table with a half-baked solution. I've done the side-by-side deal with many junior folks on my teams and sometimes it has been quite depressing at the stuff I'll find.

Answer 3

A good review start is to write the checkin comments before the reviewer get there, that way review startingpoint is WHAT code needs to accomplish and not how its done - if code review does not catch the essence of what coder wants accomplished then its a waste of time.

Answer 4

I tend to disagree with pair-programming being a complete replacement for code reviews. I think there is real value in having someone look at code with the mindset of reviewing it and having no ownership to it. If you are pair programming, you are both looking at the code thinking about what must be built, not what is wrong with what you built.

I think reviewing should be done from a checklist of items that in a perfect world are relevant to the code you are working on. The reviewer should go through the code on each check and look for the issues that pertain to that. For example, if they are looking at security issues they will look for common mistakes made in code that causes security issues. What I like about this is it again focuses the reviewers attention to one class of problem and the code is looked at in that context.

Answer 5

I agree with the pair programming idea, however, you can also switch in a different person for a second code review on commit.

To keep it streamlined, the presenter should be ready to explain:

1. The problem/requirement that created the need for the code.
2. An overview of how that code was implemented.
3. How other systems are known to interact with the areas the changes touched.
4. Any technical problems that were encountered and how they were overcome, including reasoning behind any decisions made.
5. Any conventions that will have to be used from then on because of the changes.

All of this should be explained up front, before looking at code. Then they should start walking through the code and review it all again.

I think this process works for a few reasons:

• It forces the presenter to think through or write down everything s/he did, which is good for seeing if what you did makes sense.
• It helps the presenter get better at organizing knowledge, which improves several technical and non-technical skills.
• It gives the reviewer time to think about the problem itself, separate from the code.
• The code then annotates the ideas for the reviewer instead of creating them, which means s/he can spend more brain power thinking about whether or not the changes were correct instead of thinking about whether the for loop is terminated correctly.

Answer 6

We use perforce as our source control system and as soon as files are checked in we get notification about what code was checked in and one of the senior developer looks at the changelist and reviews the code and sends feedback.