Code reviews

TenCommandments-50Back when I was an intern (well, the British equivalent) for a software consultancy I was taught how to develop software properly. I’d already figured out that I needed to design up front as I developed ever more complex software for my home computer – but some of the other techniques were less obvious.

Teleca was a small company where most of the engineers had come from large software consultancies. They made a profit by minimizing salaries and maximizing the rate they charged – just the same as any consultancy. To do this projects usually ran with a ratio of 2-3 junior to 1 senior engineers on each team. It’s a good model for software as a large amount of work involves filling in the details once somebody has designed the overall architecture and outlined the structure of the code.

But how to take these junior engineers and make sure they write quality code? If you’re going to charge them out at a hefty rate you want to make sure that they know what they’re doing, even if they’re not yet experts. Well, there were templates for functional specifications, technical specifications, we had design reviews, coding standards, unit tests, soak tests, regression tests, acceptance tests – the full set. It can be tedious but luckily we had strong team leads who made sure the processes were followed.

The problem was that the process only got us so far – we still had dumb code ending up in the system from inexperienced developers like myself.

The solution came in two parts. One was a serious bit of fun – the Teleca Commandments. I think it probably started out as 10 but ended up at 99. This was a mix of coding standards (e.g. thou shalt check parameters on entry to a method) and project standards (e.g. man shall not check in code that he has not tested) which were easier to remember than the slightly lofty coding standards. The other solution was the Friday afternoon code review.

A basic printed form was distributed – just the name of the reviewer, a list of the modules or files examined, and a box for any comments – and people in the office would exchange code with either a more senior member of their team or just another engineer in the office. The easy thing was to point out any basic violations of the Commandments.. I still can’t bring myself to use literals in code due to shame of having them pointed out by the reviewer every time. Just as easy to spot (but were occasionally justified) were so called code smells:  a method that is much too long or appears to do more than one thing; a class exposing some private data as a getter or as a parameter; two classes which appear to be working as one; poor defensive coding; overly clever algorithms, etc. The harder part was understanding why something was done the way it was – but here is the greatest opportunity for learning. Why did you make this method synchronous – is it a performance hit? Why did you open the database here – is it a security risk? Why is this a linked list? Why did you use malloc()/free() instead of a static buffer? I remember Ralph introducing me to ring buffers during one of these reviews – a technique that has always made me question how data is stored and aged in any system.

I’ve seen many different forms of code review since then – some teams review all check-ins, some teams review all changes once code hits production, but teams practice pair-programming and are peer reviewing constantly. The proximity of our code reviews to our Friday pub lunch was probably not chance. Regardless, my code improved dramatically as a result of this feedback – comments were never snarky, never mean, always well intentioned and delivered with an understanding that improving each engineer helped us improve as a company. In addition, think it helped remove the veil of secrecy that some engineers like to keep around their code – the idea that “my” code is somehow private.

So, if you want to quickly develop the coding skills of your more junior team members – or teach your old dogs some new tricks – at the same time as improving overall project quality then code reviews are the best way to do it. Start this week!

Advertisements

2 responses to this post.

  1. Code reviews are great for improving the flow of knowledge and increasing understanding around a team. On my last project we started using Review Board ( http://www.review-board.org/ ). Not only did this allow us to track that reviews had happened it provided a historic record of discussions. Recommended.

    Reply

  2. Posted by Paul Sherwood on August 4, 2010 at 7:43 am

    seems this currently is the only internet reference to the Teleca Commandments – either another example of us failing to market properly, or proof that our secrecy-based sales approach worked 🙂

    there were always 50 commandments (unless you count the final … when all else fails, make friends).

    code reviews are important – but they work best in the environment you’ve described (ie where honesty and improvement of engineers is valued). no amount of reviewing will help if the engineers aren’t intelligent and motivated.

    was very surprised to learn that Ralph used to know what a ring buffer was 🙂

    Reply

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

%d bloggers like this: