Implementing Peer Code Reviewing
June 29, 2006
At my daytime job we are in the process of implementing a peer code reviewing process. These are the sites I used as guidelines and the process we ended up using. Of course, as always the process will need evaluation and tweaking.
Articles of interest
- Code review – Wikipedia, the free encyclopedia
- epowiki: Code Reviews
- Source Code Review Guidelines
- Michael Swanson’s Blog : Code Review and Complexity
- Code reviews are too late, design reviews are better
- Effective Code Reviews Without the Pain
- Code Review Terminology
- Code Reviewer’s Guide
- How to use code review processes and tools
- Source Code Review Process – AEP Practices
- How Closely is Open-Source Code Examined?
- Ten Commandments of egoless programming
- Macadamian’s Code Review Checklist
- Why code reviews are good for you
What we ended up doing
Code reviews are done to find errors in the code and to spread knowledge in the organisation so that you can learn from your co-workers and they can learn from you. They are not meant to show off to your colleagues. They are “Code Reviews” not “Developer Reviews”.
Types of reviews
We’ve split the process in a design review part and a code review part. They are both different processes so I’ll further describe the code review part.
Code reviews are done on your own. You sit at your desk in select some code to review. The comments are written down in a document which is then send to the person whom’s code was being reviewed. That person is given a day to read and think the review over. This doesn’t mean he can take a day to “consume” teh review, it just means he can take some part of the following day to “consume” teh review. If he has any remarks or questions then he is sitting together with the reviewer to discuss those.
What is being reviewed
Code reviews have to focus on algorithm correctness and code design issues because checking those is harder to automate. It’s not so much about applying the correct prefixes to variable names, typing errors, capitalization of class and variable names, etc… As a good programmer you must be applying that part of the source-code standard, and applying it should be easy because it is very well documented, or so it can and should be. So we focus on correctness and code design first and on “text standards” second.
Whats all this type of design stuff?
I’m not a firm believer of “Big Upfront Design” (Does anybody still do? And if someone does, do they dare to admit it?). But I’m not a believer of “Emerging Design” neither. The art is to balance both. So in the upfront design you modularize your application, describe the majority of classes you are going to need and how they interact. This basically corresponds to the UML package diagram, the static class diagram and the dynamic interaction diagram. This gives someone examining your code an overview of how you accomplished the specifications. This upfront design is reviewed during the design review. But I am aware that you can not anticipate all problems in your upfront design. So you will have to do some design while you’re coding. And this latter design is reviewed during the code reviews because having a design review meeting for this is just to time consuming.
There is also what I reffer to as “code design”. That’s stuff like how to implement an exception, what is allowed to be exposed from a dynamic code module (e.g. a windows DLL). This is also reviewed during the code review.
To assist in what to review we have a list of questions, grouped by subject. Dependent on the time at hand, you pick one or more subjects and “pose the questions”. I will not sum the groups and items here, because they are dependent on your application/project. Example groups and questions might be:
- Exception handling
- Is the accompanying text clear ?
- If transforming exceptions, is the original exception passed on as a child of the new exception?
- Multi-threading issues
- Are there any deadlocks ?
- Resource allocation
- Are there resource leaks?
When reviewing code modules, we also look at the coherence of the module: should the classes in the module be in this module, or should they be in another module, or maybe a new module.
What code gets reviewed
Selecting what code to review is simply done by selecting an implemented feature or solved bug. So we don’t review “some classes”, but we review a feature or bug. This way we can also review the dynamic aspect of te code, and not just simply pick some classes at random and review them.
Another way to select code is by examning code metrics. In time, we will install an automatic code metric tool to identify “hot spots” in our code. I hope we will then be able to more judiciously select the code to review.
We prefer low frequency and long reviews above high frequency and short reviews. Of course, we don’t review a complete day, but I think that one hour is a bit short for a review. You have to “get into” the code, understand the code, and that requires some time. So to give that time we prefer less frequent reviews which take a bit longer.
What to do with the results
Changing code or correcting the problems is done by the author of the code, not by the reviewer. I firmly believe that you can learn from your errors only by correcting them yourself. The developer can of course be assisted by the reviewer in certain cases. I can imagine some code written by a junior developer needing changes which need a senior developer to assist the junior. It is a sort of mini pair-programming session (No, whe’re not doing pair-programming as we are an uneven number of developers ).
- Use the output of code metric tools for choosing areas to be reviewed.
- Implement automated standards checking (The naming conventions stuff)
- As always, tweak the process…
version 1 – June 29, 2006
version 2 – August 10, 2006