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.