Code Reviews: Lessons Learned
Updated: Feb 15, 2022
The practice of code reviews can be cumbersome but, when done right, can be highly rewarding for both the reviewer and the code author.
"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." - Martin Fowler
Code Reviews are an essential part of the software delivery process. It involves reading through code and trying to spot potential issues. The issues could be:
1. Potential Bugs
A certain condition may cause the code to fail with an unexpected error. The code author may not have covered all the test cases and will therefore have to improve code coverage and make the necessary code changes to ensure all tests pass. Using a static code analyser (such as SonarQube) helps to automate the detection of potential bugs.
The code may not be efficient and therefore may suffer from performance problems. The code author will have to refactor or even rewrite some parts of the code to achieve better performance. More stress testing may be required to verify the performance.
The code may not be written according to agreed standards or conventions. Most organisations have established code conventions that all programmers must adhere to. Ideally, a custom code formatter should be used within the code editor. This minimises the effort on the part of the code author to ensure all conventions are met. Some conventions may involve using a particular API or library to achieve something. For example, always using the Apache commons library for certain string operations. Whatever the conventions are, the code review should highlight any gaps in adhering to them so that the author can take steps to close these gaps.
The code may not be doing the right thing or may not be meeting all the requirements. A good code review should involve being aware of the exact requirements. This goes beyond merely reading through the code but also reading through the specification as to what functionality is expected. If the reviewer finds that the code does not meet the requirements, the code author must complete the implementation accordingly. This may involve rewriting or adding tests. Practices such as Behaviour Driven Development and Test Driven Development ensure that the expected behaviour is well-defined and the code is written accordingly.
Code Reviews, though an integral part of the software development process, can suffer from some common pitfalls. Some of these are:
No Context: Sometimes, when reviewing a code change, it is necessary to be aware of the wider context. This is especially true of new features or enhancements. This relates to the functionality aspect of the code review.
If the reviewer is staring at code and can see what it is doing but has no idea why, this will slow down the review as communication and discussion will be required between the code author and the reviewer before the reviewer can proceed with and complete the review.
This goes beyond checking whether the code works or is well-designed. It is more about checking whether the code is doing the right thing (i.e) it meets the functional requirements.
In a high performing team, context will be provided as part of the process. For example, Github workflows allow the code author to provide a detailed description of the commit. This can link to specifications that indicate the wider context and the rationale behind the change.
Delays: If particular expertise is needed to review the code, this can result in undue delays as the person required to review the code may not be available. Ideally, a team should never run into this situation. Any code should be able to be reviewed by anyone in the team. If not, it indicates a gap in sufficient knowledge-sharing among the team resulting in an expertise bottleneck. An undesirable side-effect of delays is that the committed branch may become stale. When the code is eventually reviewed, code conflicts can occur when attempting to merge the branch due to outdated code.
Rush: On the flip-side of code reviews being delayed is the problem of the code itself being delayed. This can result in the tendency to rush the code review as there is an urgent need for the code to be pushed through to production. In this situation, it is the responsibility of the reviewer to ensure all the usual checks are done and to make it clear that there won't be any shortcuts. If the code does not make it to production on time, so be it.
Ideally, a well-organised team will never run into such a situation. Work items including 'urgent' bug fixes should be scheduled so that enough time is allowed for careful implementation and a proper code review. It may mean that team members may have to stay back to get things done. A good organisation will reward such efforts by allowing the team members involved to take time off in lieu.
In order to avoid the above pitfalls, it is recommended that teams pro-actively ensure that these are avoided by being:
Prompt: Once code is ready for review, the assigned reviewer must make their best effort to get it done as soon as they can. If they cannot immediately come out of what they are doing, they can get to the nearest milestone and then take a break to do the review. Ideally, work items should be scheduled in such a way that all programmers have enough time to review others' code.
Collaborative: More often than not, code reviews will involve discussion. Unless it is a very obvious change, any changes suggested by the reviewer will need to be put forward and sometimes discussed as to the best way to implement it. Some teams prefer talking it out, others prefer using tools such as Slack. Whatever the preferred method is, the code author and reviewer should ensure there is no communication gap, thereby avoiding any delays in completing the work item.
Focused: Code that implements a focused change can be reviewed in a more focused manner. If the code being reviewed is trying to cover various different requirements, it takes much longer to review and is more likely to require a change following a review. The discipline of breaking down items into small focused changes has a direct impact on the efficiency of coding and code reviews. Therefore, the team must ensure that tasks are broken down sufficiently. Agile processes such as Scrum encourage this approach.
Once code reviews are done efficiently, the benefits from them will have a great impact on the overall productivity of the team. Some of these benefits are:
Knowledge sharing: The more code is reviewed, the more knowledge is shared among the team. If a particular programmer tends to work on a particular aspect of a project, having the code reviewed by as many people as possible will ensure there is no expertise bottleneck and it becomes easier for that aspect of the project to be worked on by someone else if necessary.
Break from regular work: By reviewing work done by others, programmers get a break from the monotony of their own work, especially when they have been working on a particular aspect over a long period of time. This can have a positive effect on their motivation.
Different perspective: Both code authors and code reviewers can benefit by looking at other people's code as it gives them a different perspective on thinking about problems and their solutions. This can also result in new ideas going forward and can have a positive impact on how future work is implemented.
Code Reviews are one of those things that take time to perfect and largely depends on what works for a particular team. Sometimes this depends on whether the team is co-located or not. In any case, it is important for the team to understand what works best for them and tailor their process over time to achieve optimum efficiency.