Tag Archives: engineering

What is the point of code reviews?

In most of the jobs I’ve had peer code review was an essential and regular part of the software development process. My experience is that it improves code quality and is well worth the effort. I’d also say that at least half of what I learnt in that time was through code review. Either someone would suggest improvements to my code, or I’d discover new ways of doing things from reviewing other people’s work. It’s a great way to share knowledge. This is not only true in specialist domains where the answers are not always easily found by searching the web, but any time someone is not aware of a better way of doing things. You won’t search for something if you don’t know it exists.

That’s my opinion, but there is also plenty of evidence that code review improves code quality, helps find bugs early and ultimately saves money. The numbers vary between studies, but finding bugs early is not just cheaper, it’s significantly cheaper.

What about testing?

No doubt some people will say that the purpose of unit testing is to find bugs early or, in the case of TDD, prevent them ever being created. So why do we need code review as well?

I agree that unit testing is powerful, indeed I’ve used TDD thoroughly to tame some seemingly intractable problems, but I still highly value code review. I think testing and code review achieve different things. Both are important.

Unit tests are great for quickly checking that everything that worked previously still works after a recent change. They provide confidence to refactor or experiment with code in the knowledge that the essential functionality can be quickly checked and rechecked. Achieving this via a code review would be slow, boring and error-prone.

Code reviews on the other hand can ask bigger questions, like:-

  • Is the code understandable?
  • Are the unit tests testing the right requirements?
  • Is there a more efficient way to do this?
  • Does it increase unplanned technical debt?

In fact, any check that can’t be automated. Coding style, static analysis, spell checking etc can all be automatic and the most a human reviewer should do is check the results.

Code review suggestions

Others have already written good things about how to conduct code reviews without annoying people or what tools you should use, so I won’t repeat that. Here are my observations and suggestions about how to get the most out of code review:-

  • The most important thing for a code review to check is whether the code is functionally correct. Does it do exactly what it says on the tin? Are the code and tests fully implementing the requirements of the story/ticket? Or has the developer misunderstood what’s needed in some way? If so then they’ve probably written the unit tests with the same misunderstanding, so they all pass fine. Yes, this is a hard thing to check – it takes some time to fully understand, but it is important. Get this wrong and nothing else matters.
  • Code review is a great way to share knowledge. Again, this does mean you have to take the time to properly understand the change, but it develops the team and mitigates risk.
  • There should be no reviewer hierarchy. Different perspectives are useful. More than once a more “junior” developer has reviewed my code and asked a “naïve” question only for me to scratch my head and say, “You’re right, I’ve done that totally wrong”. Even when it doesn’t happen quite like that, reviews tend to stimulate questions and the transfer of ideas.
  • That said, it can be worth making sure the local domain expert is one of the reviewers of any important change, but they don’t have to be the only reviewer.
  • Applying more engineer hours to the review does find more bugs, but each additional hour will be slightly less valuable on average than the last, so there’s a balance to be struck.
  • Don’t get bogged down in matters of opinion. When it comes to how to make code understandable and maintainable, there’s a big slice of judgement involved. “I wouldn’t do it that way” is not sufficient justification for raising a comment. There’s a danger of getting into “tomayto/tomahto” arguments. Be open-minded, take a step back and think about how important your point is.
  • Following on from the last point, it’s OK to have no comments. It might seem like you haven’t done your job as a reviewer, but it’s better than nitpicking for the sake of feeling productive.
  • If possible, do not manually check anything that can be automated. Style guidelines should be agreed across the project and checked quickly and automatically. Reviewers are human beings; they’re above that stuff and anyway they’re not very good at it.
  • Pair programming can function as a kind of instant review, but there are also risks. The reviewer can be too “close” to the code to be impartial or may make the same false assumptions about the requirements. A slower or more timid engineer might not want to question the other’s work without the time to think it through properly. So, while pairing can find bugs very early, I don’t think it should be the only kind of review a change receives.

A stitch in time saves nine

A non-technical description of technical debt

In software engineering, “technical debt” is a feature of an application’s code which makes future changes more difficult.

Technical debt takes many forms, but it can be as simple as methods with misleading names, outdated documentation or large sections of repeated code.

dash_gaugesAs an analogy, imagine that an indicator bulb fails on the dashboard of your car. You buy a new bulb and, pop out to replace the old one. Your spouse warns you that dinner will be ready soon. “Don’t worry,” you say, “it’s only a ten minute job”.

When you climb into the driver’s seat however, you can’t find any way to open the dashboard; in fact there are no screws visible at all. So you open the bonnet and look for suitable screws there. The screen wash reservoir is in the way, so you carefully remove that, but due to the way the pipe is connected, this means spilling all the washer fluid on the ground. Your spouse calls that your dinner is on the table. “Nearly there.” you reply. Now you can see some screws which look like they might release the dashboard, but they’re completely inaccessible. The only way is to get the engine out, so you replace the screen wash reservoir and drive the car around to a friend who has an engine-lifting winch. Luckily it comes out without any nasty surprises and you undo a screw labelled “DB release” – simple!

Inside the car however, the dashboard is not at all released and you notice one of the wipers is hanging loose. You re-attach the wiper and locate the correct screws. Remove EngineFinally, the dashboard opens! But it’s not over yet – the bulb you bought, after consulting the car manual, doesn’t fit. As the car is still in bits you take a taxi to the car dealer who supplies you with the correct bulb. When you vent your frustrations, he explains that the bulb you had was for an earlier model and when the new car was designed they weren’t given time to update the manual. Apparently the dashboard is made of a single piece of solid plastic because that makes manufacturing easier and cheaper. No one ever checked the screws were labelled correctly as the release date was brought forward to compete with a car from another manufacturer. Maintenance costs are usually down to the end user and rarely feature in reviews…

So eventually you get the bulb replaced, the engine back in and take the car home. By now your dinner is not just cold, it has things growing on it and your spouse is threatening divorce.

The car in my analogy has terrible technical debt. The “interest payments” needed to modify or fix it are huge. But in other ways it might still be a good car – good handling and fuel consumption and it’s black, everyone likes black. To anyone who has never opened the bonnet, the car is fine – it works, so what’s the problem?

If the occasional hassle of changing a bulb doesn’t seem like a big deal, bear in mind that software is changed a lot. No sooner is an application released than new features are needed, bugs may be discovered or perhaps an external system changes that requires the application to change to keep up. A code file is created once, but will likely be changed hundreds or thousands of times over the lifetime of the application. Most of these changes are not anticipated from the outset and the cost of making them can be dramatically affected by how well-designed, well-documented and testable the code is.

Sometimes code is made well in the first place – that takes an upfront investment of time. Sometimes existing code is improved to be more maintainable – the time to improve it is like paying off the debt. In either case the investment saves time down the line.

There is more on writing maintainable software here. For a deeper look at technical debt, read Martin Fowler’s piece.