Code review checklist

As I was discussing in my previous rant, a code review should consist of 200-400 lines that are looked over in an hour or less.

The point of the checklist is to be S.M.A.R.T. that is

  • Specific – the points are specific enough to say YES or NO.
  • Measurable – you can “sign off” and say that you did the review, and people can see if you did (you can even store the ‘results’ as a completed check-list)
  • Agreed – the whole team working on the codebase can look at the list and understand and agree
  • Relevant – relevant to the code at hand. C# specific, relevant to high performance, multi-threading, items relating to sql statements or pooling DB connection. If the system changes focus then you need to remove the items that are irrelevant.
  • Time-boxed – the checklist should be short enough so that you can remember most of the items (actually for me, it is too long; i think i could only really remember about 5-7 items) and complete them in a reasonable time

It would be possible to create some regex-based code checks to help with things like this, but they would only be helpers. You can’t fully automate all of code-review. And note that this is for code review only; that is, the nuts-and-bolts of implementation notwithstanding previous comments on there being No design but the code. The interaction of code review and design review and agile practices like pairing.. well that is a whole other cake for a whole other day.

So, the list:
For C# only, not for design review, CODE REVIEW ONLY

(not in order of importance)

  • Are the sections of code (i.e., methods, structs, classes, enums etc) of small enough to be reasonable?
  • Does the class wrap unmanaged resources? if so ensure that it implements IDispose correctly?
  • Does the code contain anything “dead” – i.e., empty interfaces, methods/classes that aren’t called, inheritance hierarchy that isn’t used?
  • Correct exception handling: no empty catch, no generic exceptions, no losing of stack trace, no losing inner exception
  • If the uses try..finally see if they could be replaced with Using blocks
  • If classes have explicit finalizers, ensure that they are necessary and do no evil
  • Goldilocks principle should be applied to logging, not too much, not too little.
  • Static members should be checked for thread safety
  • Every public method should be justifiably public, not just by accident or for testing.
Advertisements

3 thoughts on “Code review checklist”

  1. I really like your C# checklist.

    So often people have checklists that are too long or that could be automated with tools — neither one of those works for humans.

    Your list captures things that only a human being can determine and which directly relate to code quality or maintainability.

    Thanks! We’ll link to your article from our website.

  2. Hi,

    Quite a good post.

    I am sharing a link where a Quality Analyst has shared some Code Review Checklist.

    For example:

    Data Item Declaration Related:
    1.Are the names of the variables meaningful?
    2.If the program language allows mixed case names, are the variables names with confusing use of lower case letters and capital letters?
    3.Are the variables initialized?
    4.Are there similar sounding names?(For unintended errors)
    5.Are all the common structures, constants and flags to be used defined in a header file rather than in each file separately?

    For more detail here is the link:
    http://www.mindfiresolutions.com/Code-Review-Checklist-238.php

    Hope you find it useful and of assistance.

    Thanks,
    Bijayani

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