Pull Request (PR) reviews are a critical aspect of collective code ownership. PR reviews reveal to your coworkers, your problem solving methods, thought process and coding style. Pull Requests also give your teammates insight into the problem you’ve been tackling and potentially visibility into a part of the code they are not familiar with.
Making a pull request can be empowering, but it can also make you feel vulnerable. Exposing your hard work to others can be intimidating. Will it be accepted? Will others think my code is garbage? Am I smart enough to even be making these types of changes? Whether you are working on an internal or open source project, just remember that we’ve all been there before. The projects and teams that you want to be part of will welcome your contributions, and will work with you by giving you meaningful feedback. Most PRs don’t get merged without a comment or request for a change, don’t get discouraged!
When it comes to reviewing PRs, your role as a developer changes from “problem solver” to “solution reviewer”. It’s troubling how this seemingly simple switching of hats causes some to take on a completely different personality. Of course, there are also those that show their true colors, for better, or worse. I’ve found that negative PR reviewers generally fall into one of five types:
- The Know-It-All
- The Comment-Golfer
- The Machine-Gun-Sprayer
- The Manual-Linter
- The I’m-Gonna-Get-You reviewer
Photo by Chase Clark on Unsplash
The Know-It-All goes into extremely long comments to deliver their diatribe on a particular topic. It’s usually that person that doesn’t miss an opportunity to rant or ramble during a team meeting. Sometimes there are useful nuggets in the wall of text, but those can be hard to extract. This person usually means well, but comes across as “I’m better than you” and leads to a lot of frustration because the comments are difficult to respond to.
On the other end of the spectrum is the Comment-Golfer. Code golf is a competition where “participants strive to achieve the shortest possible source code that implements a certain algorithm” (via Wikipedia). Imagine earning points for leaving the shortest possible comment. “Use reduce” or “singleton pattern”. Or maybe you won’t even get actual words, and you’ll just get links to blog posts (apparently PR comment-golf doesn’t count links as characters). So you’ll see a link to a stackoverflow answer, or a Martin Fowler article and have to try to figure out the intention of commenter. These types of comments usually benefit from more context and a splash of sentence structure learned in 3rd grade. (Side note: I’m a big fan of Fowler’s writings, my point here is that context is everything)
The third type is what I call the Machine-Gun-Sprayer. This is another person who thinks someone is keeping score, and that each comment is worth a point. They will post a dozen comments on your PR pointing out the same thing over & over (i.e. “You should use map instead of forEach” or “Need a space here”). Combined with the Comment-Golfer, this person racks up points in a game where, unfortunately, everyone loses.
The fourth type is one you probably recognize: the Manual-Linter. This is the person that points out all the inconsistencies your PR has with her preferred coding style. In fairness, sometimes it’s the team’s agreed-to coding style, but they’re the ones that aren’t codified in linting rules. Instead of taking the time to research and write a lint rule, the Manual-Linter prefers to leave comments about spacing, braces, indents, new lines and “margin: 0px” vs. “margin: 0” .. on every single PR!
Photo by Adi Goldstein on Unsplash
The fifth, and most toxic type is the I’m-Gonna-Get-You reviewer. This is the person that no matter how big or small the PR, always leaves negative, sometimes condescending comments. This happens because the reviewer does not trust, or worse, doesn’t “like” the person who made the code change. This is unacceptable behavior, especially in a corporate/team environment. Unfortunately it’s not so simple to mitigate. These types of comments can divide a team and cause completely unnecessary distress amongst it.
The worst part of these personas is that most of them would be upset if the tables were turned and someone commented on their PR in the same way! It’s mind-numbingly simple to just leave comments that you would appreciate people leaving on your Pull Request. It’s the old saying “Treat others the way you want to be treated”. Nothing new here. This is kindergarten stuff.
How do we deal with these personas?
While there is no one-size-fits-all solution here, there are some things that can be done to make reviewing and receiving comments on Pull Requests a little bit easier, and friendlier.
I propose 3 solutions to handling these personas
- Collaborate more
- Dedicate time
- Make better PRs
Collaborate more
In my experience, the Know-It-All usually comes from a good place, they want to help. Encouraging this person to get involved earlier (grooming/planning) or to pair program is essential. This not only helps the Know-It-All imbue his thoughts and knowledge, but spreads domain knowledge to the rest of the team. The Manual-Linter may not know how to write a lint rule. Take it upon yourself to sit down with them and work through it together. The benefits of codifying rules into an automated system should be obvious. Just be sure the team agrees with the changes, or urge the Manual-Linter to stop leaving comments that don’t have the support of the team. I am a huge proponent of automation. Automate everything you can. If you have face strong resistance against certain rules or tools, talk them through. Find out what’s behind the resistance and try to find a compromise. Be incremental, start small. Anything is better than having a live, breathing human checking for semicolons and whitespace!
The I’m-Gonna-Get-You reviewer is a bit unique in this list. This one isn’t going to get solved by tooling, but by communication. Before teammates become sworn enemies, it’s important to talk. I’m not suggesting couples counseling, but something pretty close. Without broaching the topic, it will only escalate and drag down the entire team. As a team member, I expect that you’ve picked up on this behavior either through witnessing it first hand, or hearing about it from concerned teammates. Either way it’s important to get the two sides talking. If you aren’t comfortable approaching either of the people involved, engage your team lead. Involve management only if necessary, but certainly do so if you feel the situation is out of control, the resolution isn’t to your satisfaction or the problem persists.
Dedicate time
Part of the problem may be that not enough time or priority is given to reviewing your teammate’s code. The Comment-Golfer and Machine-Gun-Sprayer may just go in for the “quick win” which unfortunately leaves a bloody trail of teammates behind. This is an easy way to make it look like a proper review was done (“Look at all these comments!”). A reminder that “this is your code base too” along with allocating enough time are potential ways to resolve this part of the problem. Personally, I like to do my PR reviews first thing in the morning (before most get in), and know some people literally block their calendar to do it. These are usually team leads that have the most knowledge of the system and can help see patterns and reduce duplication. But, remember: this is a team effort, so everyone must do their part.
Make better PRs
Another substantial part of the problem can be attributed to the PR itself. Reviewing code is not a simple task, it requires a basic understand of the goal of the PR and knowledge of the system. So guess what happens when you drop a 34 file, 1200 line change on your teammates? The same thing that happens to you when you see it … you groan! Smaller PRs are easier to review, and while they tend to get more comments, it’s usually better to be heavily scrutinized than ignored. It’s 100x better than the blind “Approve” click that typically accompanies a large PR.
Besides “smaller”, PRs should also provide enough context for the reviewer, who may not know what you’ve been working on, to quickly understand the scope of your change. This could be as simple as a link back to the original defect or story. If you are changing the UI, screenshots are always appreciated - a before & after comparison can go a long way. And don’t be shy, if you aren’t sure about something, comment on your own code! I’ve found it extremely helpful to comment on my own PR with something like “I’m not sure this is the best way to do this”, especially on a function or a loop that I was able to make work, but just felt less than ideal. Making these kinds of comments shows that you tried, and shows that you are open to suggestions from the reviewers.
Conclusion
Writing code is hard, reviewing other people’s code is even harder. We’ve looked at different types of problems that can arise during Pull Requests and potential solutions to resolving them. Next time we’ll take a look at what goes into making a good PR comment and provide some examples.
What other personas have you witnessed in the PR review process? How did you and your team handle those situations?