How to: Code Reviews

I did a presentation on Code Reviews to the rest of our development team this morning, so I reckoned I might as well share the contents here :)

So, what's a code review?

Here's wikipedia's definition:

A code review is systematic examination (often as peer review) of computer
source code intended to find and fix mistakes overlooked in the initial
development phase, improving overall quality of software and can also be used as
a tool to better develop skills at the same time.

I actually think that's an awesome definition, but rather long winded. Here's the 'Jo Translation':

Somebody looking at your code and finding your stuff-ups – you might even learn
something!

There's a few other things that stand out in my mind as defining code reviews:

• They serve as a 'consistency check' over a project - making sure we're all doing things the same way, and following what a colleague of mine likes to call the 'design vision'.


• They're one way to make sure that someone else has a vague idea of how your code works - making sure that the project will go on even you get hit by a bus tomorrow morning.


• They're a chance to raise any issues or concerns you have - one-on-one time with your team lead, make the most of it! "I'm not quite sure how to approach this...", "Is there a better way to do this..." or even "I'm feeling really stressed right now"


• They're a learning opportunity, not just for the reviewee, but the reviewer as well. There have been a number of occasions when I've been reviewing someone else's code and have really liked the approach they've taken to accomplish something complex.

How do you do a code review?

If you can't review everything, start by figuring out what is most important to review - functionality which is critical to the system or code which is particularly complex. Once you've figured that out, sit down with the reviewee and get them to walk you through (and talk through) the code in question.

I think the easiest way to do this is: get them to close all open files in the IDE, collapse all projects in the solution view and then go through things one at a time. I know it seems petty, but it keeps things really clear and forces you to go through things sequentially and not jump around. Sometimes the very act of creating a 'blank canvas' like that puts the person into 'review mode' and can help prevent things degenerating into a simple chat about where things are at.

I'll usually try to track through a particular piece of functionality, which means that reviews tend to start at the UI and track back through the layers. "The user clicks this button, which fires this event, which is handled by this, which calls this, which goes to this web service..."

I can't over-emphasise the importance of talking through code at this level. Have you heard of the 'Teddy Bear effect'? Apparently the name comes from a university who once put a teddy bear near the help desk in the computer science labs. Students had to explain their problem to the teddy before they could ask the tutors about it. Why? Because the teddy often solved their problem.

I remember one particular student from my time as a lab tutor who would frequently come up and tell me at length about some issue he was having, only to finish with "Oh, yeah. That'll work. Thanks!" All without me saying a word...

There's something about putting the inner workings of code into words that forces us to really think through what it's actually doing, as opposed to what we think it's doing.

Anyway, back from the tangent. When reviewing code, start at a high(ish) level of design (i.e. method level) and look for things like:

• Any un-necessary duplicated code, indicating something that could be abstracted out


• Error handling


• Glaring security holes


• Unit tests - not just that they exist but that they actually cover something of value

Ask questions: Can that ever be null? What if the web service is down? Does it handle leap years? What if...

Then check for lower level bits and pieces:

• Adherence to coding standards


• Readability


• Consistency


• Commenting


• Hard coding (there shouldn't be any!)


• Sensible' code - i.e. nothing that could end up on The Daily WTF

You can also check things like logging, auditing, concurrency, performance, data sensitivity, caching, you name it. If you can think of it, and it's important to your project (or is your pet peeve) and is checkable quickly: check it. Why not?


Do code reviews early and often. They're especially important at the beginning of a project (or with someone new to the team) when you're still establishing your 'vision'/approach/whatever. Also, make sure they're done before you release anything! There are two ways to decide when to do them:

• Schedule a review regularly - this is what I do. I use weekly for most people, fortnightly later in the project and twice weekly for new grads or anyone I'm concerned about.


• Review whenever someone tells you that a task is 'done'. I haven't tried this myself yet, but an experienced colleague recommended it and I like the idea. Might try this on a future project :)

Make sure that your code review time gets included in the budget right from the start. Half an hour per person per week is probably fair - it will be more than that at the start but less at the end of the project so should average out nicely.

Why bother with code reviews?

Hopefully by now this is obvious... but just to recap:

• We're all human, we all miss things and make mistakes. A code review can pick up some of those before they hit anyone else.


• Code reviews help solve the 'our critical person got hit by a bus' problem because someone else has an idea of how your code works.


• They help ensure consistency over a project, which helps make maintenance easier, which drops the cost of maintaining software, which makes your clients happy.


• It's a great learning opportunity for both reviewer and reviewee.


• When they're done right, code reviews improve software quality enormously.


• A scheduled code review provides and opportunity to discuss any issues or concerns.

That last point has been really important to me lately - trying to lead a team of ten developers it has been hard to keep on top of where everyone is at. Scheduled weekly code reviews gave me some quasi-uninteruptible individual time with everyone to catch up and make sure everything is going smoothly and there are no road blocks I need to deal with. Since this project has been more than a little hectic it also gave me time to just sit with the team to soothe or encourage and keep things moving. Invaluable.

How do I get me one of those?

If you're a team lead and aren't doing code reviews right now - start.

If you're anybody and aren't getting your code reviewed right now - go bug someone. Your team lead is a good candidate if you have one, but if you don't, pretty much anyone will do. It doesn't have to be someone more senior than you, although more experience does help. The only real criteria for a reviewer are:

• Someone who is willing to give you advice - i.e. they're not too scared of you and aren't just going to say "That looks fine... You're so wonderful... Please promote me now..."


• Someone whose advice you will listen to - you need to respect them enough to take their advice seriously and be willing to change your code as a result.

And that's it... happy reviewing :)