Drive-by Code Review

I decided to write a blog post about code review. It is something I get very passionate about when I see it being done incorrectly and it frustrates me that it has become a tick-the-box exercise for some.

I call it Drive-By Code Review. This is where you grab the guy next to you for "a quick two-minute code review" and nothing constructive happens...you just get to check your code in with a nice comment that it was reviewed by X. What is the point in that?!

Of course this can go to the other extreme too. Formal reviews with groups of people that take hours where every line of code is scrutinized to death...normally this is too much but of course has its place in places where the smallest mistake could be fatal (literally...)

I have read some interesting articles on code review recently and I would like to offer my cut-down lightweight process that I would like to see implemented where I work. I think these steps are generic and can be applied to any team but we are a .NET team that uses a CI server along with StyleCop and FXCop so I have mentioned these as part of my process.

1. Explain context of the change I am reviewing.

Every change should be tracked to a bug or feature or ticket or story or whatever is the driver for this change.

2. Show me your unit tests.

This obviously allows me to see that you have actually written unit tests but it really allows me to see your code from the consumer of the code's perspective.

3. Show me your code.

Your code should do what it needs to and no more. It should follow the coding standards of the team.

4. Show me zero StyleCop warnings and zero FXCop violations.

No broken windows here - every warning or violation of our rules should be treated as an error and fixed.

5. Show me all this building on the Continuous Integration server.

Our builds should fail if there are any violations of our rules or any unit test fails.

I believe if our code passes these tests and gets this far then it is in pretty good shape to be handed over to our QA team for system testing, integration testing etc.

One other important point I feel is worth mentioning is that if your change impacts on any other component or library etc. then somebody who has knowledge in that area should be part of the code review. This just makes sense to me.