The Importance of Code Reviews
A few years ago, as a group of inexperienced developers at my company, we transitioned from the absence of code reviews to requiring them. Before the transition, we were still operating above a minimal baseline of quality, and we were absorbing the cost of many small (and a few large) failures. As the engineering team grew, we knew the quality of our work would continue to suffer if we didn't hold each other accountable.
We needed to be better than this. Much like misunderstandings in the use of spoken language, developers can make mistakes when expressing their ideas in code. A coding language's build process is good at catching typos. Automated tests do a great job validating and revalidating assumptions made by developers. Beyond this, we need a fresh perspective and a more sophisticated strategy for ensuring code correctness. We need code reviews.
For some time, I've wanted to express the importance of doing code reviews. Intentionally, this is not a code checklist. This is an overview of personal code review concerns.
Don't know what all the fuss is about?
If you're curious about code reviews and want to get started quickly, my recommendation is not to worry about code review tools. "Asking someone else to look at your code" is all a code review really is. The fancy tooling around tracking code reviews is just to reduce the friction of collaboration and give us a paper trail to trace back old discussions.
Choosing code reviewers
As the developer is preparing code, there is an opportunity to reflect on which teammates to include in the code review. With regards to the amount of support needed to verify the work, it's best to set expectations as early as possible within the team.
It's good to loop in senior engineers on higher-risk changes to increase the likeliness that mistakes and consequences related to the code changes are identified.
It's good to loop in multiple engineers on higher-risk changes to spread responsibility. Spreading responsibility brings the individuals on the team closer together and promotes the idea of sharing successes and solving problems as a team (not as individuals).
It's good to loop in newer engineers on lower-risk changes (possibly with support from another engineer) to get them familiarized with the code base and have them provide meaningful contributions to the team as soon as possible.
It's good to loop in fewer engineers on lower-risk changes to avoid generating too much low-impact feedback and maintain overall team productivity.
Nitpicks
We can think of nitpicks as low-impact, opinionated feedback. It's the type of feedback that passes dangerously close to a personal preference and has little consequence on the quality of the code in the sense of maintainability and extensibility. If the code reviewer is delivering nitpicks, it's likely that the code reviewer has already identified the most important feedback and is "further refining the code changes".
In general, code reviewers should timebox their code review sessions. Long code review sessions can instill a need to find and provide feedback for the sake of feeling productive. Once the impact of code review feedback lessens, it is best to move on to other work and maximize time spent delivering high-impact contributions to the team.
Does the work meet the acceptance criteria?
Acceptance criteria are statements describing what a new code change should deliver. Often, they represent a mix of…
- user-centric behaviour of the product (the value delivered to users of the software)
- non-functional qualities of the product and the code (the value delivered to engineers working with the software)
- "implied acceptance criteria" set by the team that apply to all deliverables and are often geared towards long-term productivity (test coverage, ease of deployment, telemetry, alerts, and more)
The reviewer looks over the code and confirms (to the best of their ability) that acceptance criteria are completely covered by the implemented features. If possible, a list of implemented tests can help map the behaviour of the code to acceptance criteria. Even better, if tests are automated and integrated into the code approval process, we can continually revalidate that the code meets the expected behaviour.
Feature demonstrations
Even if the higher-level feedback might not be directly actionable and tied to a line of code, feature demonstrations go hand-in-hand with code reviews. The biggest risk in the development process is making incorrect assumptions about what our code should do, and developing functionality and tests adhering to that flawed vision.
Setting up a demonstration of the features to stakeholders before finalizing the review process is highly recommended. Stakeholders (notably, the product manager) are able to confirm that implemented features behave properly from the perspective of end-users. Engineering leads are able to confirm that, among a multitude of solutions that can present the same behaviour to the user, we chose the solution that maximizes productivity for engineers by balancing speed of feature delivery and code maintainability.
The Big Picture
Engineering teams must often take action on incomplete information and start putting together the building blocks of a greater solution. Such is the embodiment of an agile mindset. Code reviews serve as a checkpoint to not only solicit feedback for the code change itself, but to solicit higher-level feedback on whether the code change itself fits well into the greater solution.