Moving Towards an Effective Yet Supportive and Inclusive Code Review Culture
How different contexts can improve code reviews
Writing code may be one of the craziest things I do.
It’s hard, and sometimes, the result surprises even me, the author. After all the hard work of getting everything factored, working, and passing tests, I’m elated as I proudly look at the result — another small masterpiece — until the code review comes.
Reviewing code and getting my code reviewed became an integral part of my developer experience. Depending on which side you’re on at a given review it can be a great opportunity to learn, teach, or problem-solve together. On top of that, code reviews are well-established practices for upholding and improving our code — and code is the internal quality of our products.
But I’ve also experienced the kind of code review that was ineffective and, on occasion, stressful, frustrating, and all around — a missed opportunity.
Fortunately, there is a fix — and it starts with the context.
“There are few if any context-free solutions, but many valid context-specific ones.” — Dave Snowden
What We Look for In a Code Review?
We might have different expectations from a code review, depending on the context. Participants and their roles, the type and size of the feature, and which stage of the software development lifecycle (SDLC) we’re in, can help us understand which context we’re in.
Different Contexts of a Code Review
Here are the different contexts of a code review and what to look for in each:
Problem-solving
Use the power of the group to devise and evaluate the best solution for your problem. This is especially effective when you are in the early stages of a new design or planning to refactor for new functionality.
Challenge: Intimate understanding of the problem is needed to find subtle issues within the solution. Pick your partner wisely.
It can easily be confused with defect finding, but it’s not the same.
In defect finding, we aim to discover bugs and defects in our solution, but problem-solving is about finding the most appropriate solution in the first place.
Defect finding
Now that an appropriate solution path has been chosen and the first lines of code have been written, it’s time to pause and see if you can discover bugs and defects in the solution. The solution is reviewed to ensure the overall factoring makes sense, the right abstractions were chosen, and core algorithms can perform what is needed.
Challenge: Deep domain knowledge and understanding of the problem space are needed to discover subtle defects, bugs, and other quality-related issues.
Maintaining norms
When talking about norms, we discuss maintaining organizational preference. These things may not be essential to maintaining a healthy code base and include code formatting, common idioms, and styling.
Gatekeeping
Contrary to the “shared code” ownership that dilutes responsibility, I am a strong proponent of the “your code is your code” view. This means that each team has one or more repos they own, and each part of the repo tree is owned by someone known as the code owner.
For a code owner, any code coming from ‘outside’ (usually outside your team) is something to stay aware of. We want to protect our code from outside contributions that might not be experts in our domain.
Important: this goes beyond readability.
When reviewing code in the gatekeeping context, we look for things that do not align with our design principles, compromise the language of our service, or break the conceptual wholeness of our design.
Accident prevention
When near or at ‘code completion,’ we rarely seek big, design-altering inputs from a code review. But we still would like another pair of eyes to ensure we didn’t miss test corner cases, for example, or find bugs related to concurrency, etc.
This may sound like ‘defect finding,’ but it happens in a later stage and with a much-reduced scope. Here, the demand for deep knowledge and understanding of the problem is reduced; instead, we rely on high technical proficiency.
Education
When reviewing code with a new team member, for example, we would like to introduce them to the complexities of our problem space and explain the idiosyncrasies of our code base.
While performing the code review, we emphasize education and use this opportunity for mentoring, learning, and disseminating knowledge.
Readability
We want to ensure we only accept clean and coherent code. Therefore, a readability review is the time and place to enforce and ensure any submitted code follows these objective standards.
Challenge: Everyone has their own perception of code cleanliness and readability. This means that code is often reviewed and accepted based on subjective opinion, creating inconsistencies in quality, coherence, and cleanliness.
To prevent this, create team and organization-wide style guides. These guides will be the objective definitions of clean and readable code. Based on these guides, you can objectively assess the submitted code.
Notes For Reviewers
It is not your code
Sometimes, you may become a co-author of the change, especially for bigger features or early in the development cycle. But, in the context of a code review, please remember that the reviewed code is not your code but the author's.
The implications here are:
- Comments in the spirit of “I would do it differently” rarely serve any purpose
- When the author disagrees with your suggestions, consider if they may be right. Even if you are not convinced, remember it is still their code.
Own the comments
Comments and requests that you open as a reviewer — should only be closed/resolved by you. If you feel pressure from the author to close comments, ask yourself if it’s because they feel you slow them down.
We do not want to hold the PR ‘hostage’ by our comments.
While you should insist on some things because insisting is, sometimes, the only way to fix something that otherwise won’t be fixed, the following behaviors may help you be more efficient:
- Be respectful
- Focus on the ‘Why’
- Be objective (refer to authoritative sources as much as possible)
- Be fast
Speed
Even the best code review will come under pressure if it takes a day to arrive. Answering clarification questions an hour or a day after submitting makes a world of difference. Delivering timely and fast reviews is a sure way to alleviate many of the usual complaints about strictness, etc.
Be fast.
For Change Authors
Choose reviewers wisely
A good reviewer has the following qualities:
- Regularly works on the relevant code
- Has relevant domain knowledge
- Framework/language experts
The best reviewers are usually code owners. Often, they can give you the most thorough and correct review.
Preparing your code for review
- Consolidate TODOs: try to leave as few TODO items as possible.
Only real dilemmas should be left. - Set the context: give a good description and link to a ticket to explain the problem and the proposed solution.
- Make sure you’ve cleaned the commit history (git rebase and squash)
- Check that all tests have passed
Resolving comments
Avoid resolving/closing comments or change requests unilaterally.
Yes, it is your code, and sometimes you will find yourself under pressure to approve this PR now, but treating the reviewer comments as a barrier to PR approval, instead of seeing it as valuable input to improve work — is a sure way to devalue the entire process.
Getting to LGTM
It is the code authors — yours— responsibility to get the code approved so it can be merged. This has several implications:
- Do not fire and forget. Actively seek reviewers and make sure the code is reviewed
- Fix it. If reviewers say they do not understand something in your code, work on clarifying your code, comments, documentation, etc.
- Resolve conflict by trying to reach a consensus with the reviewer. They are on your side
Improving the Design of Your Process
Focus on convergent practices.
When working on smaller features or stable code bases
- Feature small, localized changes (the bulk of a change should usually be in one file/class)
- Short-lived branches (1–3 days max)
- 90% of reviews should not need more than two reviewers (most should do with one)
- Pair program with someone who is a qualified reviewer for the code so your code is reviewed as you go along
- Lean into in-person reviews where the reviewee can walk the reviewer through the code and the reviewer can ask questions online
- During the review, focus on education, readability, maintainability, and accident prevention
When working on big features or greenfield projects
- Start reviewing early
- May have longer-lived branches (up to two weeks)
- Attach supporting documentation
- During the review, focus on conceptual integrity, good design, mutual problem solving, education, defect finding, gatekeeping
- Keep some architects/leads for the first several iterations
Flow
Every big task design starts with a design review. A big task is anticipated to take two or more weeks.
- Big picture: High-level look at the initial design. Very little to no code, mostly reviewing documents. The focus is on understanding requirements and preparing questions for the following reviews. Reviewers and contributors are architects and stakeholders.
- Intermediate designs: A series of reviews that use partial code. The code here is mostly scaffolding. Usually, we would like to design one or more POCs here and review them. Reviewers and (at least partly implementers) are architects/tech leads.
- Early code review: Looking for subtle design flaws and errors. Reviewers should have a good understanding of requirements.
- Code review: The flow is similar to the ‘stable/legacy systems’ flow.
Thanks for reading. Stay tuned for more!
