Designing Awesome Code Reviews
--
There are many tools available to software engineers that help improve code quality. Unit test and mocking frameworks make it easier to write tests. Code coverage tools identify where more tests may be needed. Lint tools and static code analyzers help catch things engineers may have overlooked.
While these tools are great in freeing up an engineer’s time, they are in no means a replacement for code reviews. These tools cover only a part of what happens in a code review, as code reviews are far more than finding syntactic errors, identifying missing test cases, or following the coding standard. Most importantly, a well executed code review doesn’t just improve the quality of the code – it also makes the team better.
At Trunk Club, we strive to make code review a collaborative learning experience for everyone involved – not only for the reviewer and the author of the code, but also for anyone who may just be reading through the code review. Code review is not just about being right or wrong. It is about understanding the author’s approach and the implications of the code, transferring knowledge, and being collaborative. A well designed and documented code review process makes these goals and expectations clear to everyone.
Inspirations
When designing the code review process at Trunk Club, I took great inspirations from design critiques. In a design critique, designers present their work to each other and share constructive feedbacks and questions. Design critique and code review have the same goals — avoiding personal preferences and criticisms, delivering constructive feedback, and emphasizing collaboration and learning.
Because they share the same core fundamentals, the same concepts and guidelines that apply to design critique can be applied to code review. I have taken some of these design critique concepts and applied them to the code review process at Trunk Club, which is shared below.
Criticism vs Code Review
While the words may look similar, critique is different from criticism. Likewise, code review is different from criticism. Criticism is personal. Code review is about the code. Tanner Christensen’s distinction on critique vs criticism can be applied to code review as well:
Criticism passes judgement — Code review poses questions.
Criticism is personal — Code review is objective.
Criticism is vague — Code review is concrete.
Criticism tears down — Code review builds up.
Criticism is adversarial — Code review is cooperative.
Criticism belittles the person — Code review improves the code and the team.
Language Principles
Code review is typically done in writing, and having clear written communication is helpful for all parties, even for future parties that may not be involved at the time of the review. It is through this written communication that the thought process of both the reviewer and the author is exposed and transferred. Therefore, articulating constructive feedback well is vital in making the code review a collaborative learning experience. Using consistent tone and language helps avoid any miscommunication or misunderstandings.
Tanner Christensen’s design critique method of asking questions can be applied to code review to make it more collaborative and constructive:
Code review should primarily be presented in the form of exploratory and guiding questions, with an intent of building up and improving the work and the team through collaboration. Posing thoughts as questions allows the author to express the reasonings instead of being defensive.
And Braden Kowitz’s approach on keeping design critique objective can be applied to help code review be less personal and be more about the code:
Avoid “you” and “your” to focus on the code and the architecture. Affirm what’s working well and why. Discuss the problem first, and then suggest the potential solution. Make suggestions, not mandates, and trust the author will explore a solution and make a good decision.
Feedback Format
Even with the guidelines above, it is helpful to have concrete examples and specific formats on how to construct feedback that reflects these principles. The following two formats, which are commonly used in design critiques, are helpful ways to give constructive feedback and guidance that embodies the above principles.
What/how format:
- What are some ways to make this code more readable?
- How can the code be optimized to parse the JSON only once?
- What happens when X happens (edge case)?
I like/wonder/wish format:
- I like how this is flexible and makes it easy to test.
- I wonder what the advantage of looping through the array every time is.
- I wish the JSON can be parsed only once since it’s expensive to read the file every time.
These formats, while very helpful, are not perfect. Always remember the core principles of asking exploratory questions and not making it personal.
Avoid you/your:
- Why did you do it this way? What is your reason for this?
- I wonder what you were thinking about.
- I wish you didn’t have to parse this JSON multiple times.
Avoid yes/no questions:
- Is there a better way to do this?
- Did you intend this? Was this intentional?
- Should this method be broken up?
Rewording Feedback
Below are some concrete examples of rewording feedback.
“This will crash when it’s null so you should null check this.”
- What will happen if this value is null? What are some cases where this value might be null?
- I wonder if this value can be null and what might happen in such a case.
“Should this be a constant instead?”
- What are some benefits and side-effects of having this value hardcoded?
- I wonder why this value is hardcoded and if there are any downsides to hardcoding it.
“Can you remove the duplicate code and make it generic by doing X?”
- How does this duplicate code affect maintainability? What are some ways to make the code more maintainable and reusable?
- I wonder if this duplicate code will continue to be maintainable. I wish the code was more reusable.
Responsibilities
To make sure the team is consistently getting better and learning through code review, code reviews should be executed in a consistent format regardless of who the reviewer or the author is. A clear and shared understanding of responsibilities reduces any misunderstanding of what’s expected of the reviewer and the author.
Reviewer’s responsibilities:
- Make sure both the reviewer and the author understand why and how the implementation works / might not work.
- Understand the reasoning behind the author’s implementation, regardless of whether you agree or disagree with the implementation.
- Guide the author to think about the why and how by asking questions, as opposed to making direct statements or prescribing solutions.
- Suggest reference materials as needed — examples, documentation, blogs, Wikipedia.
- It’s ok to say “everything looks good” when everything is good.
Author’s responsibilities:
- Review your own code and look at your own diffs before submitting code for review. Be confident about what you know and what you don’t know. Understand and explain any compromises you are making.
- Explain why and how the implementation works at a high level. Simple outlines or diagrams are very helpful. Timebox this effort to keep it short and high level.
- Give enough context to the reviewer. Where does this fit in the big picture? What components are involved directly before/after?
- Point out specific areas of concern, if any, and explain your concern and your approach. These may be edge cases or components you don’t understand that well.
- Ask questions if you don’t understand the reasoning behind the reviewer’s feedback.
- “I don’t know” and “I haven’t thought about it yet” are perfectly fine answers. Take some time to think about the feedback and explore. Don’t be pressured to have an immediate answer.
- Acknowledge the feedback, make a decision, and explain your decision. You don’t have to address everything at once. If you decide to address some feedback separately, clearly explain the decision and how and why you made that decision.
Code review request templates (PR templates on GitHub) are helpful in making some of these a consistent part of the code review process. You can collaborate with your team to outline specific questions or sections on the template, which will be filled out for code reviews. This will go a long way to ensure the above responsibilities and expectations are met.
Common responsibilities:
- Make it clear what you’d like the next step to be — Is it ready for another review? Do you want feedback from a specific person? Do you want to take another look?
- Review comments can go only so far. Get together and pair to ask more questions, explain further, and explore together. It doesn’t have to be a long session, whether it’s pair programming or whiteboarding.
Focus on Engineering Concepts
Focus on how and why the code works, and how and why the code can become better. The how and why should be based on software engineering concepts, principles, and best practices.
Concepts to consider:
- Single Responsibility Principle
- Separation of Concerns
- Open/Closed Principle
- DRY
- Stability
- Performance
- Monitoring
- Testability
- Dependencies
- Side effects
- Verification
Avoid Monolithic Code Reviews
Keep the code review in mind and plan for it. The code should be modular and the merges should be small logical modules. Try to keep the code review changes under 500 lines.
Ask for feedback if you have trouble breaking up your code. Be empowered to break down and rewrite big user stories into subtasks or multiple stories. Don’t try to build a whole feature at once. You are already writing the code a little bit at a time, so plan and set a good stopping point for the code review. Even before writing any code, it is helpful to collaborate on planning for code review, to break up your work and share the context.
Refactor to Unblock
Refactor incrementally to unblock. Avoid refactoring unrelated code. Keep the refactoring scope small and local to the new code.
Refactor or suggest to refactor only if the newly written code introduces more workarounds or adds more tech debt.
If the refactoring adds complexity, break it up into a separate code review. Handle the refactoring first if needed, and then build the code on top of it. Ron Jeffries has a great post about how to fit refactoring into the development process.
Closing Thoughts
Code review is a powerful way to make the team better through learning and collaboration, and it should never be criticizing or personal. However, code review is not the only way to be collaborative. The collaboration and learning can happen at every step of the development process, through whiteboarding and discussion.
Code review should be the last step of the collaborative process, not the first step. Proactive whiteboarding and architecture discussion, writing user stories together, planning for the code review, can all happen collaboratively before writing a single line of code. With earlier collaboration, there won’t be any big surprises by the time your code gets to code review.
Do you have any examples you’d like to share? Do you have any code review comments that may be challenging to reword? Let me know in the comments below or tweet @hiBrianLee!
Some of my other posts:
