By Bruno Skvorc

The Importance of Code Review

By Bruno Skvorc

Every developer knows the pain of banal mistakes. A wrong attribute here, a misspelled property there, an accidentally duplicated line of code which you missed because of the coffee-fueled 16 hour hackathon you’ve been on. Even a simple $ before the opening PHP tag which you accidentally put there because you started typing before the infernal Java-based IDE warmed up and re-positioned the cursor can leave you scratching your head for hours on end if you’re tired and distracted. If only you had a fresh pair of eyes to look at what you did – surely these mistakes could easily be avoided?

Wikipedia defines code review as follows:

Code review is systematic examination (often known as peer review) of computer source code. It is intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers’ skills. Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.

This definition is as precise as it gets – in even more layman terms, code review is simply the act of having someone else look at your code to find the mistakes you missed.

The Types of Code Review

As the Wikipedia definition hints at, there are many different ways to review code for defects. Here’s a quick break-down of a few of them:

  • OTS (Over the Shoulder) Review – This is how small teams usually handle code reviews. A developer will write a decent amount of code and call another developer over to have a look at it. The other developer sits there while the first one explains what he did, line by line. Through this narrative, the original developer notices some of his own mistakes and fixes them, and the OTS developer notices others, pointing them out to the first. They also share opinions on solutions to certain problems which the original developer will sometimes redo after the review process is done, calling for a review once again. This can also be easily done with screen sharing software and voice chat if the developers are remote.
  • Tool-Assisted Review – there are various tools both online and offline to assist with code review. While a detailed look at the various tools on offer is outside the scope of this article, we can generalize and say there are paid versions (Atlassian Crucible, CodeCollaborator), free versions (Review Board) or, if you’re solo developer, the community version (Stack Exchange Code Review). Regardless of the tool used, each serves pretty much the same purpose – it retrieves the most recent changes to source code and flags them as needing review. A peer – that means a developer of equal or greater skill – then reviews the code, flags it as reviewed or marks any errors that were found and makes suggestions, and either finishes or reinitiates the process by sending it back to the originating developer. It is also important to note that many popular IDEs have code review plugins.
  • Pair Programming – A very dynamic type of code review, pair programming is a hotseat “game” for two developers in which one developer codes and the other follows his progress by sitting beside him. After a couple hundred lines of code or after they reach a predetermined milestone, they take a short break and switch places. The one who was coding now observes while the one who previously observed now codes. This is extremely effective in avoiding bugs and improving overall code quality, but it costs twice the manpower. Many companies are not ready for such a risk and are unfortunately unable to think in terms other than “two people on two machines do more work than two people on one machine”. It is precisely this type of review that yields the best results: not only are bugs avoided flat out, but the two developers directly collaborate and share ideas on solutions to problems they encounter as they progress. It is also worth nothing that this type of review is incredibly difficult to implement in teams that aren’t used to it – it mostly works on younger teams.

As an aside, there is also a formal type of review, first introduced and researched by Michael Fagan in the 1970s (this method is also known as Fagan Inspection), now somewhat archaic and out of favor in the industry. Formal inspection is rarely used in small teams and mostly pertains to several-million dollar products as it is very mentally intense and expensive. It includes several people (up to six) sitting down with a projector and reviewing code together. Each participant is assigned a role (such as Reader, Moderator, or Reviewer) and when the team notices bugs or defects of any kind, everything is detailed to a great extent – from severity, to the actual line of code, to the cause and effect, even the predicted cost of this bug reaching the customers. This is by far the most professional type of code review, but also the most discouraging to developers and as such not widely adopted. Studies have shown other methods of code review to be just as effective if used right, without the need to tie down that many people for so long.


Best Practices for Code Review

Once it’s been decided to implement code review, there will probably be some hurdles to overcome. Management may not see justification for the extra time review takes, or some programmers may think a review is a personal attack against the code they worked hard to create. Here’s some tips you’ll do well to keep in mind when code review is implemented.

  1. Know your common mistakes and actively fight them. When working, every developer has errors he commonly makes no matter how deep “in the zone” he is. Each and every one of us has this little glitch that is just silly and noticed outright by others. Take note of these slip-ups. Forgot to filter input… again? Forgot to comment a method… again? With a list like that, a developer can actively hunt down those mistakes before calling for a review. This is known as the ego effect – you know your code is about to be reviewed and you don’t want to hear the reviewer saying “Aw man, you forgot to filter input again!” You want to be the rockstar, the ninja, the person who makes others say “Wow, that’s actually a great solution”. The ego effect is what will drive you to improve your code before others even have a chance to take a look at it.
  2. Peer code review means being reviewed by someone of equal or greater skill. As should be common sense, code review cannot work when a junior reviews a senior’s code. The junior might notice some usual discrepancies, breaches of standard, typos, or even errors like input filtering, but will usually be unable to identify a bigger problem. This will often require the definition of a hierarchy by skill in the team if one is not already present.
  3. Less code with clear milestones means better reviews. Code should be reviewed only after a personal milestone has been reached, and these milestones should be small and should happen often. In object oriented programming this is especially important, but also especially doable. Finished a new component which extends an interface which ties into an adapter that’s already been reviewed? Finally fixed a problem with a specific method that’s been bugging the department for a week? Great! Component by component, and keeping it up to a maximum of 700-800 lines of code at a time (comments inclusive) is what produces the absolute most efficient review process. Keep the code you need reviewed short, concise and independent and do the review as soon as possible after finishing while the ideas are still fresh in your mind. Just remember – time is expensive, so don’t take too much of it either! An hour should be more than enough to perform this “section review”.
  4. Collect metrics. This is easier if you’re using a more structured type of code review like tool assisted or formal, but can be done in OTS as well. Try to note the number and types of bugs and slip-ups you find through a given number of lines of code and units of time. Aggregate this data across developers and easily find out who needs the most help, who breaches the most standards and how much money you actually saved your company by doing code review. Soon you’ll be able to actually quantify the usefulness of code review, and only then does everyone in the company become actively intrigued by it. This is usually the most difficult part of a review to implement, as few developers have the patience for manual statistics, but it can be beneficial if adopted.
  5. Be mindful of the social aspect – finding bugs is good, not bad! It is imperative to remember that finding bugs is good, not bad. Never stigmatize or single out a developer who made a mistake, make it informal. Have people adapt to code review – help your peers accept it, not fear it. Code Review can not only be useful – it can be fun as well. Remember that more time and more difficult tasks equal more bugs – the number of bugs does not indicate a developer’s skill! If you’re a manager or team leader, make sure no one sees code review as negative or a waste of time. Make sure they don’t see it as a rule enforced by the company but as an improvement to their everyday workflow which they should keep regardless of the company they work for. Whenever possible, strive for a personal approach of code review. If you’re using tools, use them in OTS as well. The personal approach of mutual brainstorming and informal discussion of potential solutions to problems is invaluable in the process of accepting code review as your friend.

In Conclusion

Code review can be incredibly difficult to implement, especially in a team of old-timers who aren’t used to it. But once done, it will not only decrease the number of defects in your code, it will also increase collaboration and help team building, improve “brotherhood” amongst developers and will propagate best practices and improvement of skill across an entire team or department. Any type of code review is better than none, so if your team isn’t using it today, suggest it. It can only help. If you’re a solo developer, find a kindred spirit to review code with – go online, socialize, expand your development circles, team up. Don’t look at other developers as competition and at code review as your enemy, look at others like brothers in arms with code review as your weapon on the front lines to perfection.

Image via Fotolia

  • Rabih Younes

    Hey, recently got a chance to try out the trial version od Review Assistant, new code review tool, now thinking of upgrading to the Pro edition, but still doubting, may be i should opt for Crucible? Review Assistant performs really well and it a pleasure to use. Have you heard of it?

    • I can’t say that I have, thanks for the heads up though, definitely worth checking out!

  • In my experience, “old timers” are more apt to do code reviews :) One huge and mostly overlooked benefit of the formal review, where all developers gather, is not in the quality of the code under review, but in the knowledge sharing as more experienced people point out better ways of doing things.

    • Good point. Although, it’s very difficult to balance the benefit milking seniors for experience and the fact that six or more people aren’t working at the time. I guess it depends on the team’s setup. 5 juniors and a senior definitely makes sense as a formal review group, while 5 seniors and a junior, less so.

  • Nirav

    Really great article. Code review is very important and helpful. Keep on sharing such things.

  • Pyone

    Don’t look at other developers as competition and at code review as your enemy, look at others like brothers in arms with code review as your weapon on the front lines to perfection.

    100% agreed with you. Thanks for the great article.

  • farhad

    Writing unit tests and doing TDD (test driven development) would make code review pointless since the unit tests would capture everything you wanted to do. I think the very concept of a code review by “looking” at the code is useless. The brain simply cannot interpret code the way a computer does. And Obvious mistakes would mean careless or hasty programming and again, would make the unit tests fail.

    • I disagree. TDD will eliminate bugs, sure, but only those you actually remembered to test for. It won’t help you optimise your code, keep it consistent with other developers’ workflows or make sure you follow standards. It won’t think of alternative approaches to complex queries and will only look at results – while TDD will make sure your code *works*, code review will make sure it works *well*.

  • fullsupp

    I don’t know which planet of the Solar System, Bruno came from. Me, I came from Saturn. We always do code review there so we make sure we do not step on the codes of other programmer. Code review is a must and an old old thing. Older than Bruno and myself.

  • I would like to point out, that even if you work solo, and, in a team, even before let’s say “committing/submitting” your code… you can and should do a code review of your own code in your process. I know some people will say it is biased and some people just cannot see their own mistake. That is probably true for some people and to some extend, but in reality, there are a lot of people being able to do that and benefit from it.

    Of course someone might not get the benefit of peer feedback about how to do it better from an external source (including from “equal skill” developers). I still believe that skilled developers can improve their technique doing that.

    And for banal/common mistakes, they should be easier to spot on when looked separately from the main development process and since it is your code, it should be faster to go on. This should encourage you to develop good habits too when coding (if you are responsible for the code, you’re the one that will scream about that lack of escaping/filtering/naming/etc. and you should develop habit to avoid those as much as possible).

    After finishing a development block, someone can and should just look at his whole output just like when we proof a text.

    For me it is common sense and it might be for your too, but you might be surprised, even among lot of “professional”, the amount of people who submit code without testing, reviewing, etc.

    That being said, mistakes will arise and that is normal. A team should work together, and people should work toward to make the whole team more efficient. Review-wise, you have to evaluate the efficiency-ratio with the time you code, test, review, etc. and try to improve your method to get more and more efficient (and usually that make your job “easier”, unless that make you inherit of “harder” jobs!)

    • Agreed, self-review should always precede calling another developer to look at your code. It saves everyone’s time, and inspecting 50-100 of your own lines at a time wastes no-one’s time.

  • hpoom

    Good article.

    I don’t agreed that Pair programming is a type of code review. Pair programming is very different to code reviewing. In our current business do both pair programming and what you describe as Over the Shoulder code reviews. Where we pair program a third person, not one of the pair is needed to do the code review.

    I am also not sure about “Peer code review means being reviewed by someone of equal or greater skill”. We allow juniors devs to review senior devs code. We have in all the company’s I have worked for. Juniors learn from this and can still spot mistakes. Only if it is major feature or change would I insist on a senior dev reviewing the code.

    • Thanks for sharing, I hadn’t encountered juniors reviewing a senior’s code yet. Sure, we make the juniors *look* at the code, understand it, and go through it fully, but we don’t count their analysis as a code review. We’ll accept the bugs/typos they find, certainly, but before it goes into production I believe all code should be checked by a senior.

  • ColemanTO

    I’m looking forward to using Sonar at work:

Get the latest in PHP, once a week, for free.