.net C# Code Review Checklist

I had to do code review on quite a few projects. Interestingly, I wasn’t able to find a good and reliable “Code Review checklist” for .Net/C#, so I compiled my own. Now I’d like to share it, it may help professionals looking for something similar.

Let’s clear out some basic context.

The work is done at Infomatix Ltd., where I work. We decided to publish our CR checklist and method, to make the world a better place (and on our way to world domination with code ;)).

This CR method can be greatly improved (comments are welcome), but keep in mind that other forms of QA are already in place, and I needed a reasonably small, manageable set of rules, without over regulating everything.

The real “heart” of code review is not static analysis. We use a tool for that, StyleCop, and I highly recommend to use that or a similar tool. Let the machine do what it can, the more you automate, the less you have to spend with mindless tasks. The real challenge with code review are the parts that not fully objective. Or in fact, in some case are not objective at all – beware and try not to give way for a never ending religious battle -, the rules of clean code, the analysis of “what this piece of code really does, and in what form it would be better”; and the analysis of the architectural and structural parts of the code base.

So everything that StyleCop examines is already there, this checklist doesn’t contain those items. (Examples: existence and wording of XML comments. Namespace/using rules. Usage of spaces and curly brackets. A lot of rules on naming things.)

Here is the checklist: Code Review Template

The “Rule Codes” are just mnemonics, these should be short, resembling the content of the rule, and unique.

There are items are not included in the list (deliberately), like SQL Injection or Cross side scripting.

The process is simple. The code reviews I do are not part of the everyday work of the team working on the given project, so it’s not done on a pull request. It is a bit ad-hoc, and I need to do it any time on any branch.

To keep it simple, manageable and visible to all, I put my CR findings as TODOs in code. I create a CR branch for that.

Example:

// TODO: CR X - you have an enum for this type, is the string valid here?
switch (connectionType)
{
	case "oracle":

The team will see these in the “Task List” in Visual Studio (or they can search for CR comments), and they are expected to fix the issues, or handle it some way, even with a comment giving a reason or suppressing the issue.

We (with StyleCop) require that all public (and protected) members have XML comments. On the other hand, regarding to Clean Code standards, we don’t want the comments to be meaningless. (If you let the coders give generated commends without adding anything to an already well named member, you let noise into your code.) So the rule for that is that you have the right to indicate that no additional comment is needed for the given element. Example:

[SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1600:ElementsMustBeDocumented", Justification = "SD")]
public string PartnerName;

A few words on the types of rules. Currently we have three “flags”. I felt that I needed to indicate these cases.

  • LP – Low Priority.
    This is pretty straightforward. I like these to be reviewed and noted, but they really not a big issue, they are easy to fix, so nobody needs to lose hair over them.
  • P – Project level divergence is allowed.
    Some rules are not universal, and can be “set” by the team or project. Example: how logging of errors is conducted may differ per project to fit the requirements.
  • H – Hard to analyze and or keep
    Just to indicate for both the analyzer and the responsible writing the code, that this is something more than “please add a comment here”, this may take time on both end. (Currently only one rule is marked with this one.)

You can gather different measures or notes on these rules, like indicating the number of cases or priorities. Since I give my result right into the code (with the TODO comments), I only indicate a status on a certain rule (per Visual Studio project). These are:

  • y – yes, that’s OK
  • n – rule violation(s)
  • m – maybe violated, please review
  • x – no problems found, but not thoroughly inspected
  • ? – requirement in context (project scope) is not known.
  • na – Not available, not applicable.

n is red, that’s clearly a violation of a rule.
m and ? are yellow.
The rest are green.

Advertisements

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