Code Review Checklist

Here is a plea
From my heart to you
Nobody knows me
As well as you do

Depeche Mode, Ramat Gan Stadium , May 2009, Assuming no new war in the next two months

You don’t have to do code reviews, but if you do, do them the right way.

Code review  is a managerial process, IMO, and should be done by managers, not peers. The reviewer does not inspect just the code, but also the development process, guidelines and state. For example. If a peer detects a problem requiring a change in the code, a fix will happen for the specific problem. If the manager detects a difficulty in the code, he can make sure the cause is fixed. Peer review is an interesting tool and has its own benefits, but the two should not be confused.

The great benefit for the reviewing manager is education. He learns about the real problems his engineers are facing, the real status of code, documentation and people skills.It also allows him to stay connected with the source. It is  a great trade-off point between easy-to-shout-global-always-true declarations ( “No code with printf, ever”) to the reality of day to day (“printf is ugly, but it works”).Of course, he gets to learn more about core issues  in the code worth fixing. The same issues that he never prioritized.

There are some nice guidelines here http://www.macadamian.com/codereview.htm and below are some bullets you might want to use in your own process.

1.       Was the change tested to work in real environment?

2.       How do you KNOW if does what it is supposed to do?

3.       Are there potential similar bugs in the code? Were they fixed?

4.       Is the change\feature documented in the code?

5.       Are there unit tests to check the new feature?

6.       Are there any compilation warnings? Why?

7.       Is there an open ticket for the change?

8.       Has the change been checked for security issues?

9.       Did you  check memory allocations and releases?

10.   Is the change accompanied with right debug messages and logs?

11.   Functions have clear descriptions, return values, input and output

12.   Standard type definitions are used

13.   No arbitrary constants are used in the code

14.   What are the risks involved in the change? How are they minimized?

15.   Does the reviewer fully understand what the code does?

16.   Are state machines documented as such (clear states and transitions)?

17.   How will the change affect performance? Did you run a profiler?

18.   Was the design document updated to reflect the change?

19.   Textual messages should be check for spelling and clarity

20.   How are errors, exceptions and lack of resources dealt with?

21.   Can the new functions be useful to other team members? Did you tell them about it ?

Cat Doing Code Review

Cat Doing Code Review

Tags: , ,

2 Responses to “Code Review Checklist”

  1. NoMoreHacks Says:

    I like these items and I think that a checklist is a very valuable thing. I think a lot of these items are very audit-friendly (e.g., “Is there an open ticket for the change?”) but aren’t all to do with code.
    Here is my own list:

    http://nomorehacks.wordpress.com/2008/12/11/code-review-checklist/

    which is – as I say – very focussed. I think that writing a checklist as a team is one of the most important steps; it forms a contract between team members that *defines* good work.

    No More Hacks..

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


%d bloggers like this: