Code reviews in the 21st Century

Find that Bug!

There's an old adage that goes something like: 'Do not talk about religion or politics'.  Why?  Because these subjects are full of strong opinions but are thin on objective answers.   One person's certainty is another person's skepticism; someone else's common sense just appears as an a prior bias to those who see matters differently.  Sadly,  conversing these controversial subjects can generate more heat than light.   All too often people can get so wound up that they forget that the outcome of their "discussion" has no bearing on their life expectancy, their salary, their chances to win x- factor, getting that dream date, winning the lottery, finding a cure for climate change or whatever it is they regard as important! 

Similarly, in the world of software engineering code reviews can end up in pointless engagements of conflict.  Developers can bicker over silly little things, offend each other and occasionally catch a bug that probably would have being caught in QA anyway - that conflict free zone around the corner!

Now don't get me wrong, there are perfectly valid reasons why you may think code reviews are a good idea for your project:

1. Catching bugs sooner means less cost to your project. You don't have to release a fix patch because it's has been caught in development phase - yippee!
2. Code becomes more maintable.  That crazy 200 line method that Jonny was writing with a hangover has being caughted before it has the chance to make itself at home deep in your code base.
3. Knowledge is spread across your team. There are no longer large blocks of code that only one person knows about.  And we all know, when that one person talks about taking a two month holiday everyone panics!
4. Developers make more of an effort. If a developer knows someone else is going to pass judgement on his work, he's more likely to put that line of javadoc to clarify when an exception will be thrown.

However, it would be naive to think that code reviews don't cause problems.  In fact, they cause so many problems many 21st century projects don't do them.  I think they have a place but there needs to be some thought regarding how and when they are done so that they are beneficial as opposed to a nuisance.   Here are some guidlines...

• 1. Never forget TDD 

Ensure you have tested your code before you asked someone else to look at it.  Catch your own bugs and deal with them before someone else does.

• 2. Automate as much you can 

There are several very good tools for Java such as PMD, Checkstyle, Findbugs etc  What is the point  getting a human to spend time reviewing code when these tools can very quickly identify many things the human would waste time moaning about?  I am going to say that again.   What is the point  getting a human to spend time reviewing code when these tools can very quickly identify many things the human would waste time moaning about? 

When using these tools, it's important to use a common set of rules for them. This ensures your code is at some sort of agreed standard and much of what used to happend in an old fashioned 20th century code review, won't need to happen.  Ideally, these tools should be run on every check in of code by a hook from your version control system.  If code is really bad - it will be prevented from being checked in.  Billy the developer is prevented from checking in the rubbish he wrote (when he had killer migraine) that he is too embarrassed to look at.  You are actually doing him favours, not just your team. 

• 3. Respect Design 

In some of the earlier Java projects I worked on, the reviews happened way too late.  You were reviewing code when the actual design was flawed.  A design pattern was misunderstood, some nasty dependencies were introduced or a developer just went way off on a tangent.  The review would bring up these points.  The proverbial retort was: 'This is a code review not a design a review!' .  A mess inevitably ensued.  To stop these problems we changed things so that anyone asked to review code would also be involved - in some way - in either the design or the design review.  In fact, we got much more bang from the buck from design reviews than code reviews.  Designs were of a much higher quality and those late nasty surprises stopped.

• 4. Agree a style guide (and a dictionary) 

Even with the automated tooling (such as Checkstyle, Findbugs etc), to avoid unnecessary conflict on style, your project should have a style guide. Stick to the industry standard java conventions - where possible.  Try to have a 'dictionary' for all the concepts your project introduces. This means, when code refers to them it's easier to check that the usage and context is correct.

• 5. Get the right tooling

If all your developers are using Eclipse (and happy using it) something like Jupiter makes sense.  You can navigate your way through code, debug code and essentially leverage everything the Eclipse IDE does to make your life looking at code easier when reviewing code.  However, if everyone is not on the same IDE (or the IDE is not making your life easier) consider something like Review Board. 

• 6. Remember every Project is different.

You may have done something in a previous project that worked.  But remember, every project is different.  The other project had a certain architecture (may have been highly concurrent, highly distributed), had a certain culture (everyone may have enjoyed using eclipse) and used certain tools (maven or ant).  Does the new one tick the same boxes?  Remember, different things work for different projects.

• 7. Remember give and take

When reviewing be positive, be meticulous but do not be pedantic.  Will tiny trivial things that get on your nerves make a project fail or cost your company money?  Probably not.  Put things in perspective.  Remember to be open to other ideas and to change your own mind rather than getting hung up changing someone else's.

• 8. Be buddies 

In my experience, what I call 'buddy reviews' (others call 'over the shoulder')  have worked really well.  A buddy review consists of meeting up with another team member informally every day or two and having a quick glance (5 - 10 mins)  at each other's code at your desk or their's.  This approach means:

1. Problems are caught very early
2. You are always up to speed as to what is going on
3. Reviews are always very short because you are only looking at new code since the last catch up
4. Because the setting is informal - there is no nervous tension. They're fun!
5. You can exchange ideas - regularly. 

When Tech Leading, buddy reviewing your team members is a great way of seeing if anyone on the team is running into trouble early rather than late.  You can help people and get an idea of everyone's progress all at the same time.  And because of the regular nature of buddy reviews, they become habitual and actually get done.  Something we can't say for many other 21st century code reviews!

In summary, if your project is going to engage in code reviews, they should be fast, effective and not waste people's time.  As argued in this post, it is really important to think about how they are organised to ensure that does not happen.

'Til the next time - take care of yourselves.