A code review, at its core, is a conversation about a set of proposed changes. Early in my career, I viewed code reviews as a mostly technical exercise that should be devoid of non-technical concerns. I now see them as one of the few opportunities to concurrently learn and teach while also strengthening my relationship with my peers and colleagues.
My team, Delivery, has been working together for at least six months (some much longer), but only two members work in the New York City office while the rest are spread across North America. Because of our familiarity with each other, most of our daily interactions take place via text or video chat. Code reviews are often short, but we also go out of our way to communicate when we are stating an opinion or being nit-picky.
Most software developers are expected to participate in code reviews, and yet few are offered any training or guidance on conducting and participating in an effective code review. Participants attempt to find the most appropriate solution to a problem given the constraints of time, effort, and skills of all involved. But how do we have that conversation? What does an effective conversation look like? And what are the challenges of participating in a code review, and how can you overcome them?
Whether your tool of choice is GitHub, GitLab, Gerrit, or another tool, the goal of this article is to help you get as much out of the code review process as possible.
What Are Code Reviews For?
Code reviews happen in a wide range of contexts, and often the skills and depth of experience of participants vary widely. On open source projects, for example, participants may not have any sort of personal relationship with each other. Indeed, they may never communicate outside of the code review process. At the other end of the spectrum are code reviews where the participants have daily face-to-face interactions, such as when everyone works at the same company. A good participant will adjust how they participate in a code review according to their knowledge of the other participants.
While it is important to adjust one’s communication style in accordance with the intended recipient, how to adjust is influenced by three primary factors: the purpose of the code review, the intended audience, and one’s relationship to the audience.
Identifying the Purpose of a Code Review
Code reviews serve both technical and cultural purposes: finding bugs before they’re integrated, identifying security concerns, ensuring style consistency with the existing codebase, maintaining code quality, training, fostering a greater sense of ownership, and giving other maintainers an opportunity to get familiar with the code before it’s integrated are just some of the reasons you may be asked to participate in code reviews. Make sure you know why you’re participating in a code review beforehand.
Regardless of why you’re conducting a code review, it is important to respect the purposes that code reviews serve for the codebase. If the only purpose of a code review is to check for security concerns, then drop whatever personal concerns you may have about coding style or naming patterns. Unfortunately, it is not uncommon for the purpose of code reviews to be poorly defined or non-existent. In that case, once you’ve determined that the proposed changes are necessary and add value, I’d suggest reviewing for correctness, bug identification, and security concerns. Secondary to those concerns may be overall quality and long term maintainability of the proposed changes.
Submissions: What to Include
Code reviews typically start with a contributor submitting a proposed set of changes to the project. The submission should include:
- A clear and useful description of the changes and give a general overview of why the change is necessary.
- The scope of the change.
- Areas where reviewers may want to give special attention.
- Subtleties that need clarification.
- Details that may help reviewers better understand the patch.
Depending on the complexity of the changes, reviewers may find an overview of the trade-offs the submitter made in the patch helpful in order to be better understand why the patch is the most appropriate of the possible alternatives.
Written communication about technical subjects can be difficult: people have limited time, and each of us is on a journey of confronting challenges and personal growth. In code reviews every participant has a role to play, each with its own set of objectives:
- As a writer, strive to be as clear as you can. When in doubt, be descriptive.
- As a reader, ask questions when something is unclear.
- As a reviewer, be gracious when someone uses their time to submit a patch to your project.
- As a submitter, be forgiving when your patch is not reviewed in the time frame you would prefer.
Regardless of your role in the review process, respect that others may be at a different place in their journey, and assume that all participants are engaging in the process in good faith and because of shared values and goals. The process is easiest when one assumes that all other participants are doing their utmost to help you succeed and get better.
Here’s an example of a pull request from our team where I asked for clarification, discussed my concerns, and ultimately landed on a compromise that made the submission better and easier to maintain, all while gaining personal knowledge of the subject at hand:
Example of how my team communicates in our code reviews.
Knowing Your Audience
Start by reading all the code. As a reviewer, recognize that the submitter gave their time and energy and tried to improve the product in some way. As you read and strive to understand the patch, record your questions and concerns privately so that you understand the full context before providing any feedback. As mentioned previously, make an honest effort to restrict your feedback to the purposes for which the code review is being conducted.
Prepare and submit your feedback after reading and understanding the changes. Be gracious. Try to keep your comments focused on the code and the solution it offers; avoid digressing into unrelated matters. If you see something surprising, ask questions. If you don’t have a strong history with a submitter, go the extra mile to communicate your good intentions. It’s OK to use emojis to communicate tone. Strive to begin fostering a healthy, productive relationship with this new contributor.
Your feedback in code reviews is one of the primary ways to build a community of developers eager to contribute to your project. By nurturing a strong community, you will promote a quality product. Especially for open source maintainers, an authentic, explicit “thank you for the contribution” or other nice words can go a long way towards making people feel appreciated and fostering a supportive community.
Take the feedback, evaluate it, and decide what to do next. For submitters, it can be difficult to read criticism of the code you have written. When a reviewer asks for changes, they are doing so for the same reason a patch author submits a patch: a genuine desire to improve the product. Remind yourself that feedback about code is not personal. You may decide to accept the feedback and change something. Or you may decide that there was a misunderstanding, and that some requested changes are unwarranted or would simply be wrong or add no value. It’s OK to push back.
Developing a Partnership Through Code Reviews
When there is an asymmetric level of experience between the submitter and reviewer, use the opportunity to mentor. As a reviewer with more experience than the submitter, you may choose to accept that submitter’s patch as-is and then improve upon it, contacting the submitter to let them know about your changes later. In a professional setting, such an approach isn’t always feasible. Have the conversation in the open so that observers (i.e. other readers) can learn too, but reach out for a more personal touch if the extent of feedback is becoming overwhelming in written form. In my experience, patches submitted by someone significantly more experienced than the reviewer are usually accepted as-is or with only very minor changes requested.
When you’re thinking out loud, make it clear to the reader so that they do not think you are asking for a change inasmuch as evaluating a possibility. If you’re nitpicking, explain your reasons for doing so. On our team, we often preface nit-picky comments with `(nit)`, in order to help contributors recognize these types of comments. This usually serves as a signal that the contributor can ignore that feedback if they want. Without that distinction, the nitpicks are not distinguishable from the feedback that the reviewer feels more strongly about. For all participants: when you’re unsure about something, ask, and err on the side of clarity and friendliness.
A successful code review will result in a higher quality change, strengthen the relationship between reviewer and submitter, and increase the understanding that everyone involved has of the project. Code reviews are not just a formality that require a rubber stamp before being merged; they are an essential aspect of modern software development that provide real value to projects and teams by promoting good software engineering practices.
Conclusion
Through code reviews, I’ve learned to be more gracious and more understanding about the personal challenges and technical struggles that everyone experiences. I have learned to more thoughtfully examine the trade-offs that we all make when writing software. I hope the ideas presented here can help you grow your community and increase your effectiveness.
Billie Cleek is a Senior Software Engineer on the Delivery team where he supports internal tools to provide a consistent deployment surface for DigitalOcean’s microservices. In his spare time, Billie is a maintainer of vim-go, infrequent contributor to other open source projects, and can be found working on his 100-year-old house or in the forests of the Pacific Northwest regardless of the weather. You may also find Billie on GitHub and Twitter.
