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

What we ended up doing

Why
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.

The process
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?
  • etc..

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.

Frequency
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 ;-) ).

Future improvements

  1. Use the output of code metric tools for choosing areas to be reviewed.
  2. Implement automated standards checking (The naming conventions stuff)
  3. As always, tweak the process…

History
version 1 – June 29, 2006
version 2 – August 10, 2006

About these ads

3 Responses to “Implementing Peer Code Reviewing”


  1. Hi,

    If you’re looking to tweak your process, you might want to consider these resources:

    http://blog.qualityaspect.com/2006/04/05/are-your-code-reviews-effective/

    http://lucidquality.qualityaspect.com/Overview/

    The basic idea is to use code reviews as a mentoring platform and improve not only the concrete product under development but also the skills and experience of the developers. Both can be achieved at the same time and using the same resources.

    Lidor


  2. Hi Lidor,

    Thanks for your reply.

    I’ve just read your suggested articles and find them very interesting.

    Allthough not as explicetly stated in my post, I think the main idea of your post is in the above description. The remarks of the code review can result into two people sitting together in discussing the code:

    “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”

    In my article on code standards you can read how we also try to manage this “mentoring” stuff: we incorporate best coding practices for our software in our code standard for everyone to see.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: