Rowan Twell
The Soft Parts of Code Review
Code review is an important part of any software organization, and the culture that surrounds it will shape the day-to-day experience of developers. I’ve been lucky enough to see some of the challenges that problematic culture presents on code review, manifesting in toxic pull request discussions and endless committees. I realized that different people enter into code review believing that reviewers should be catching bugs for them or reviews swooping in with backseat-engineering comments that don’t consider the humans involved.
This post isn’t about a hardline “do this for great success” guide (partially because no such guide exists), but to show you some of the less tangible parts of code review and what the consequences are.
Why review?
Reviewing the code we’re contributing to our products ensures that no developer works in isolation - Code is seen by the team and is owned by the team. Collectively, our code quality can be improved by many contributions, not a single person’s vision. The cultural impact of code review is far greater than any transactional outcome.
Code reviews set the tone for the entire company that everything we do should be open to scrutiny from others, and that such scrutiny should be a welcome part of your workflow rather than viewed as threatening.
— Bruce Johnson, “Lessons From Google: How Code Reviews Build Company Culture”
Review can be a sensitive process, but first and foremost, everyone should try to enter review in good faith, considering the people putting their code on show and the opinions of those reviewing.
👩💻 For Developers
Code is not owned by a single person - It is a collaborative effort. 👨👩👦👦
Don’t leave “broken windows” (bad designs, wrong decisions, or poor code) unrepaired. Fix each one as soon as it is discovered. […] neglect accelerates the rot faster than any other factor.
You may have written the first iteration, laid the foundations, or even designed the whole architecture - but you won’t be the last person to contribute. For this reason, everyone can benefit from a review process that incorporates many voices.
💌 For Reviewers
You are not an infallible code review linter-bot. 🤖
You are a second pair of eyes, a useful voice to help improve the quality of the code.
As Reviewers, We try to meet these goals:
- Improve General Code Quality
- Enforce Common Code Styles
- Identify code smells and discuss solutions
- Review adherence to SOLID principles / overall design goals.
- Improve Shared Knowledge inside Teams / Promote Collective Ownership.
You may be an expert in all of these areas or only a few. Know that your thoughts are still useful - you may provide a second opinion or catch something that seems invisible to others!
For Everyone
Our Code, Our Problems
Be prepared for feedback, and to give feedback in good faith. We’re all in this together - we’re trying to improve the code’s quality. There are a hundred reasons why a developer arrives at a particular solution - This isn’t a critique of a person’s skills as a developer.
You make the decisions, you shape the future.
Small quips have big impact.
Remember that discussions may arrive from factors that are beyond our control. Do you think there’s an action we can take, to help prevent this in future?
No-one is an Island
You may have opinions about best practices or The Best Way® to do something - this is great! This isn’t something that exists at face value - Be prepared to explain your reasoning.
Seek Solutions, Not Problems
Do not start with the assertion that the code is 100% right or wrong and work backwards from there.
Be clear about what you’re looking to improve and why. How important is it? Is it a hard-to-catch error or is it a genuine slip? If you need to know more - ask!
Does the solution start and end at this review, or can it be incorporated into a wider plan of action?
Everyone Should see the Value in the Review Process
No one is the sole gatekeeper of the merge commit. Both sides should see the merit in the review process, and all parties should arrive with the expectation to discuss different solutions.
Should the reviewers pay attention to a tricky piece of code? Do you want to seek feedback from a local expert? Be transparent, and open with all parties - everyone has a part to play.
A different context can frame a problem in a different light - context that may be hidden from you. Be prepared to ask questions!