Title: Professionalism in Programming #33

Is it me, or does this look familiar? Astute readers (who are old-time ACCU members) might feel a sense of déjà vu here. Back in the mists of time, the fourth ever professionalism column discussed code reviews. I recently needed to evangelise reviews once again; it's always the same old battles we fight. Different job, same problems.

I dug back, revisited, and refreshed my look at the code review process. I reviewed the material, if you like. It seemed appropriate to present the fruit of my labours for a fresh audience. We'll start with some craftsman philosophy…

How do you learn to be a good carpenter? You become an apprentice to someone. You watch them work, help them daily, gradually take on more responsibility, and learn from their advice. You don't jump in feet first without any practical ability and expect to churn out quality woodwork straight away.

We don't have a real analogue of that in the coding world, even though programming is as much a craft as it is an engineering discipline (possibly more so). Code reviews, however, do give us a little taste of that in the discipline of an engineering process.

Code reviews (or inspections, or walk-throughs) have similar effects to the open source model of software release - providing a structured opportunity for others to eyeball your precious code. Reviews encourage you to take responsibility for your handiwork. When you know that it's not just for you to look at, but it will be viewed, used, maintained and criticised by others your approach tends to change. You're less likely to make the quick-and-dirty fix that you'll never have time to revise. The accountability brought on by code review brings a greater quality to coding.

Code reviews are employed in traditional software engineering processes. They are arguably less important when pair programming, or when more than one person is responsible for parts of the code. In these situations they are still useful nonetheless.

The code review seeks to analyse a section of source code at several levels:

We look in great detail to ensure that the code is correct and of a suitably high quality. This process generates a huge list of 'must-fix' issues. Sometimes you will spot improvements that are not worth making now; we chalk these up for future experience.

A code review doesn't replace the code's functional specification review. The code is validated to conform with its specification, but the content of this specification is taken to be correct. If it wasn't then the task would be herculean! Sometimes code review comments might feed up to the specification (for example, where clarification is needed), but this is not our ultimate goal.

Code reviews can be:

Bugs are our enemy, the nemesis of good software development. We need to be confident about the quality of our work, and need to find faults as early as possible in the development process. The earlier we try to find problems, the more we are likely to find and fix.

Code reviews are an excellent tool to achieve this goal. According to Humphrey: "Students and engineers typically inject 1 to 3 defects per hour during design and 5 to 8 defects when writing code. They only remove about 2 to 4 defects per hour in testing but find 6 to 12 per hour during code review" [Humphrey97].

But code reviews do more than identify bugs; they weed out all sorts of problems. We perform code reviews to improve code quality. This includes:

We also check the code against a number of yardsticks, which include:

Apart from the obvious benefits of correct code, reviews have other useful side-effects. The cross fertilisation that comes from looking at each other's code ensures that coding style is more uniform across a whole project. A review also spreads knowledge about the inner workings of core bits of code, so there is less risk of loosing information when people leave a project.

In an ideal world every bit of code is carefully reviewed prior to release. According to the Software Engineering Institute at the Carneige Mellon University, a thorough code review should take at least 50% or more of coding time (although they do include 'personal' code review in this statistic) [Humphrey98]. That would take longer than a Real World project is prepared to invest^[2].

So as we write a system, we need to ask whether to review the code and, if so, exactly what code to review.

Bugs are inevitable, and you can be guaranteed your code contains some classic mistakes. There will be obvious flaws that you'll find quickly, and many more subtle problems that would only be spotted by a fresh pair of eyes approaching the code with no preconceptions. It's hard for the original author to see most inherent faults in their own work - they're too close to it, suffering the psychological cognitive dissonance described in [Weinberg71]. If your code is at all important (clue: it is, or you wouldn't have written it) and you care about its quality (clue: you do, or you're a disgrace) then you must code review.

Not reviewing code drastically increases the chance of faults slipping into your production software. That could spell your embarrassment, a lot of expensive rework and in-the-field upgrades - even your financial ruin. The effort of a code review pales in comparison with the consequences.

Often people make excuses to justify avoiding reviews. They say "the code's too large to review fully" or "it's too complex, no one person could ever understand it - there's no point even trying to review it". If a company can employ enough people to write a large program, they can employ enough people to review it. If the code is too complex then it desperately needs reviewing! In fact, it probably needs something a little more drastic. Well-written code is decomposed into sections that can undergo separate reviews.

Even the most modest project quickly produces a tonne of source code. For all but the most stringent development processes there simply isn't enough time to review every last scrap of code. So how do you decide which bits to review? That isn't easy.

You want to select the code that will benefit most from review. This is the code that is most likely to be bad, or that is most important to the correct functioning of your system. You could try these strategies:

The most practical approach is probably a hybrid of all of the above.

There are several ways to perform a code review:

Any form of review places the source code under the microscope - really aiming to criticise and verify it. This is not to pillory or 'get at' the author, but to improve the quality of the software the team produces. Simply having a code review is not enough. In itself it's not going to solve all the problems. You also need to make sure that you review properly. The most common setting is the formal code review meeting. There is a fixed agenda (to ensure that no action is forgotten) and a defined ending (not necessarily a time limit, but a definition of exactly what code you are reviewing, and what you're not - it's very easy to be woolly about this).

An example code review meeting procedure is described below:

Code review meetings are a high-ceremony review method. They're hard work, but they undoubtedly find many problems that would otherwise go undetected.

Other less intense review procedures exist, providing most of the benefits of code review meetings but packaged in an easier to swallow pill. Perhaps the most effective is the integration review, performed whenever new code is integrated onto a mainline code branch. This could be when:

At such a point, the code in question is marked for review, and a suitable reviewer is picked: either someone responsible for that module (the code integrator or maintainer^[4]) or a shadow (or code buddy) who is assigned to verify that author's work.

These 'gated' code check-ins are often implemented with a software tool that is integrated with the source control system. They're quite hard to arrange manually, and are usually left as a check-in discipline: you are not supposed to check any code in unless it has already been peer reviewed. This approach is quite hard to police; errors slip past in hurried last minute check-ins.

The actual review step here is usually a lot less formal than the meetings described earlier. The reviewer scans the code to check that it's not obviously broken, tests it (perhaps reviewing the available unit tests to ensure they're valid), and then authorises it for inclusion in the mainline. Only then will the code integrator migrate the verified code into the release tree. For more serious projects, or at more sensitive times (just before a major release milestone, for example) this review step may become much more stringent - requiring more eyeballs and more effort.

Since the reviewer and author needn't actually meet face to face (although it is preferable to do so) this can be considered a form of 'virtual' review process.

Code reviews require a constructive attitude - you need to approach a review with the correct mindset or it will be unsuccessful. This works two ways, for the author, and the reviewer:

We haven't yet considered what type of code will 'pass' review, and what code will 'fail'. It's beyond the scope of this article to describe what 'good code' looks like. As we look for bad code design and hunt software bugs, there are a few specific themes we can draw out. The reviewed code needs to be:

A review process is key to the production of any high quality item, and so is not solely useful for source code development. A similar review process is used for specification documents, lists of requirements, etc.

Code reviews are an essential part of the software development process. Just as an apprentice learns their trade from knowledge passed on, code reviews spread knowledge and teach coding capability. As more of a peer-to-peer than master-apprentice activity, they provide a learning opportunity for author and reviewer alike, and result in higher quality code.

Write your code to be reviewed; bear in mind that it's never just for you to read. Other people must be able to maintain it as well. The author is always accountable for the quality of their code.