I love code reviews, I really do. I like having my code looked at and receiving feedback and I like to look at other peoples to see differing styles, techniques and library functions I might not be aware of. But I always struggle to find the best way to do code reviews. So I want to take a look what you need to perform a code review and how to review the code.
Some people are just really bad at writing bug reports and work items (I’m looking at you developers) but this is the most important thing you need when reviewing the code, apart from the code. You need to able to read the original bug report and understand it then see the developers analysis and proposed solution so you know what you’re looking at when you review it. Without this it becomes extremely difficult and time consuming to redo all the work that has already been done to identify the problem.
Leave your style at the door, at the office we don’t have one complete standard, we have general rules about casing, and what not, but there isn’t one enforced style. I find it really helps to tune out to subtle differences in style, so long as it’s readable it really doesn’t matter.
I like to start with some simple things:
- Run it - Check that the bug has been fixed or the work item has been implemented. Take it through some basic sanity checks with the debugger attached, if possible, with stop on EVERYTHING enabled and see what it does. No point in reviewing code that doesn’t do what it’s meant to do. If it has unit tests run them, step through a few with the debugger to get a feel for how it’s meant to work.
- Check for build warnings - Ok that might sound petty, but I generally think warning should be treated as errors.
Is it readable – This is the most important, if you can’t read it how do you expect it to be maintained? Or you don’t understand the change enough to give it a good review. By this point we have checked that the code is working and that we understand it. Now for the hard part, looking at it. At the business end of a code review I generally look for the following things (C# specific):
Delegate Registration – Whenever I see a delegate being registered I always look for a corresponding deregistration.
- Resources Being Release – If a resource (file, db, etc) is opened, make sure it is released again at some point.
- General Exception Handling – I always look for things I know, from experience, are likely to throw and see if there is exception handling around it. Whenever there is a catch block, I look at how it is dealt with and what is caught.
- TryParse v Parse – Parsing values is always error prone, and we do a lot of it in our codebase, so I’m a stickler for correct exception handling or using the TryParse methods.
- Null Checking (or Use of a Null Pattern) – Defensive coding should be our bread and butter so this should always be present
- General WTF-ery – Anything that makes me utter those three little letters. I think this covers most points, it’s not an exhaustive list but I think code reviews are more about checking maintainability than quality, although it definitely helps with it.
When giving feedback I like to try and speak to the person first and ask them about it, remember one person’s WTF is another person’s complex issue. Alternatively, I jot a few points down in an email. I don’t see the need for a separate code review tool when other forms of communication are effective and flexible. Finally, always be constructive, it’s hard to get people to listen if your just being negative so state your reasons clearly, ask them why it’s like that and always resist the urge to say “W.T.F is going on here dude”.
I started off this blog post with a view to clarifying in my own mind how best to do code reviews, but looking back and rereading my approach I feel there is room for improvement. So I ask you my dear readers: What do you think makes a good code review?