IBM championed the code review process since a long time, in fact since 1974. It was initially defined as a “code inspection”, a formal static analysis process to evaluate if the software was full-filling the requirements. This process was slow but, even so, IBM enabled to obtain significant quality improvements in the delivered code.
The company (IBM) nearly doubled the number of lines of code shipped for System/370 software products since 1976, while the number of defects per thousand lines of code was reduced by two-thirds – Michael Fagan
Thanks to collaborative version control tools, it was possible to speed up this process, making it a daily routine process. In my team, in every single day, each software engineer reads, reviews, evaluates and comments on a colleague’s code. The ultimate goal is increase trust in the shippable code, reducing the number of possible bugs. The review process implies that the code may be updated and reviewed again. This exchange – between the author and the reviewer – should happen the necessary number of times until the demand level of quality is secured.
There aren’t specific rules on how this process should work. With the dissemination of this process, the good sense prevailed and some good practices emerged from different sources.
- Review fewer than 200-400 lines of code (LOC) at a time.
- More then 400 LOC will demand more time, and will demoralise the reviewer who will know before hand that this task will take him an enormous amount of time.
- Aim for an inspection rate of less than 300-500 LOC/hour.
- It is preferable to review less LOC but to look for situations such as bugs, possible security holes, possible optimisation failures and even possible design or architecture flaws.
- Take enough time for a proper, slow review, but not more than 60-90 minutes.
- As it is a task that requires attention to detail, the ability to concentrate will drastically decrease the longer it takes the task to complete. From personal experience, after 60 minutes of effective code review, or you take a break (go for a coffee, get up from the chair and do some stretching, read an article, etc.), or you start being complacent with the code on sensitive matters such as security issues, optimisation, and scalability.
- Authors should annotate source code before the review begins.
- It is important for the author to inform colleagues which files should be reviewed, preventing previously reviewed code from being validated again.
- Establish quantifiable goals for code review and capture metrics so you can improve your processes.
- It is important that the management team has a way of quantifying whether the code review process is effective, such as accounting for the number of bugs reported by the client.
- Checklists substantially improve results for both authors and reviewers.
- What to review? Without a list, each engineer can search for something in particular and leave forgotten other important points.
- Verify that defects are actually fixed!
- It isn’t enough for a reviewer to indicate where the faults are or to suggest improvements. And it’s not a matter of trusting colleagues. It’s important to validate that, in fact, the changes where well implemented.
- Managers must foster a good code review culture in which finding defects is viewed positively.
- It is necessary to avoid the culture of “why you didn’t write it well in the first time?”. It’s important that zero bugs are found in production. The development and revision stage is where they are to be found. It is important to have room for an engineer to make a mistake. Only then can you learn something new.
- Beware the “Big Brother” effect.
- Similar to point 8, but from the engineer’s perspective. It is important to be aware that the suggestions or bugs reported in code reviews are quantifiable. This data should serve the managers to see if the process is working or if an engineer is in particular difficulty. But should never be used for performance evaluations.
- The Ego Effect: Do at least some code review, even if you don’t have time to review it all.
- Knowing that our code will be peer reviewed alerts us to be more cautious in what we write.
- Lightweight-style code reviews are efficient, practical, and effective at finding bugs.
- It’s not necessary to enter in the procedure described by IBM 30 years ago, where 5-10 people would close themselves for periodic meetings with code impressions and scribble each line of code. Using tools like Git, you can participate in the code review process, write and associate comments with specific lines, discuss solutions through asynchronous messages with the author, etc.
I advise you to read the document for a more in-depth view of each point.
It is impossible to refute that the code review will improve the quality of the code produced, reducing the number of bugs and improving the design. There are some additional assets such as sharing knowledge among peers and mentoring more junior team members.
It’s a fact that this process, even if done in a lighter way, will take time to the project. In the review process, we have to add the author’s response time and again the review time again. In an attempt to expedite this consumption of time, I advise that, after two unsuccessful revisions, the author and the reviewer sit side by side and debate the piece of code that is causing the problem.
But it’s really important to state a phrase from IBM’s site – Slow down to go faster.
I would like to end with a note, which I have as too important. The code review process demands the entire development team to demonstrate high emotional intelligence. As a rule, programmers have a strong ego and feel they are “parents” of the code written by themselves. It’s sometimes not easy to accept less constructive comments. Totally destructive, provocative or jocular commentaries “will do more harm than good,” and may even destroy the morale of a team. It is important to follow these elements, to try to change their attitudes or, as a last resort, to remove them from the team.