Opening a pull request for code review can be a vulnerable thing. A lot of care and energy goes into writing code, but we often lose sight of this when reviewing, thinking that if we communicate the information we need to, it doesn’t matter how it’s communicated. But humans write code and receive feedback, and if humans are made to feel stupid or inferior for making a mistake or doing something differently than the reviewer, that can impact how feedback is received, and how the author will approach writing code in the future.
This is a small sample and of course only anecdotal, but I don’t think I’m alone in having felt intimidated to open a pull request:
Adrienne Domingus@a_domingus
Curious: have you ever felt intimidated/nervous about opening a pull request (either open or closed source)?01:51 AM - 04 Sep 2018
0
0
So if we accept that this intimidation is something people do feel, what can we do about it? I’d like to focus primarily on the reviewer, but there also things an we can do as authors to increase the odds that we’ll get the kind of feedback we’re looking for, so I’ll talk about some things that have worked for me there too. I’d love to hear any other ideas you have!
As a Reviewer
Reviewing code is a big responsibility — it’s through this collaboration that we ensure our codebases are functional, secure, maintainable, and more. It’s absolutely important to provide constructive feedback about these things, but I often see people lose sight of the fact the way feedback is given can impact the way it’s received and how the person receiving it is made to feel about their work and themselves.
Prioritize — consider the cost of the changes you request
There’s a lot to think about when reviewing code:
Whether it does what it’s supposed to
Security
Data model
Architecture/design — maintainability & readability
Efficiency/scalability
Whether it’s sufficiently tested
Reviewing each of these things requires looking at the code through different lenses, and how important each one is can vary a lot based on the product you’re building, the timeline you’re on, etc. And requesting changes to code at each of these levels can require varying levels of effort for the author to address. Sometimes it’s worth it. But sometimes it’s not. I’ve seen people get so caught up in dogma or correctness or consistency that they’ve asked engineers to do hours or days of additional work for little payoff. There is absolutely a time and a place for caring about each of these things —I would like to see people consider whether this is it before requesting a specific changes.
Recognize that they’ve thought about the problem more than you
This is a big one for me. It’s easy to look at someone’s code and say “couldn’t you just do X?”, when something looks more complex than it needs to be, or if they’ve implemented something differently than I would. And sometimes it turns out to be true, and the code could be simplified. But sometimes they’ve tried the implementation I’m thinking of, and it didn’t end up working, for one reason or another.
You can still give this feedback while not making assumptions about what they’ve already tried or why they made a decision they did, simply by rephrasing the feedback as a question — “Why did you choose to implement this way instead of doing X?” That allows for a more productive conversation — their answer may be that they didn’t think of it and can make the change, but they may also have a rationale, that they can share with you. If their rationale is off, you can approach it from that direction, otherwise you’ll have learned something about the problem without making assumptions!
Be kind
This should go without saying, but it so often doesn’t. Some things I love seeing when my coworkers do, and that I try to do as well:
- Give positive feedback about something specific in the code. Even code that needs a significant re-work has something good about it. It was a more complex or bigger project than the author had done before, and they got it to work at all! They used a method you’ve never seen before that was interesting or simplified things. They refactored a piece of code they didn’t strictly have to. The possibilities are endless — find something nice to say and say it!
- Check your mood — if you’re frustrated when reviewing code because you’ve explained the same thing before, or because something seems obvious to you, take a step back and revisit it later. Even if something seems obvious, telling the author about it in a way that won’t be demeaning or make them feel stupid is important, and will make them more likely to be receptive to and learn from your feedback.
As an Author
It’s not our job as the author of code to ensure we get feedback that, at a minimum, doesn’t make us feel incompetent or less than — that’s on the reviewer. But there are things we can do to make sure we get feedback that is targeted and meaningful. These are just a few thoughts on things that have worked for me and teams I’ve been a part of.
Context
No code is perfect, but some code is better than other code — but the thing that makes code better or worse is entirely contextual. So give your reviewer some context!
High level overview — what the change does and why you’re making it
How is this tested? Manually? Added automated tests? Regression?
How should this be deployed? Include order of operations for no downtime deployments if there are database schema changes, code changes, other infrastructure or config changes
Link to the Trello card/Jira ticket/issue — cards often have more context in them than is really necessary for someone to thoroughly review the code, but having a link to it can be helpful in case they’re curious, or still have open questions after reading your description.
Once you and your team decide on what things you want included in your PR descriptions, you can create a template if you use GitHub or GitLab. It unfortunately doesn’t look like it’s possible on Bitbucket yet. These are super helpful to keep everyone on the same page about what should be in a description, and makes it so you don’t have to remember each time!
Ask for specific feedback
Especially as PRs get larger, it can be hard for reviewers to know what to focus on. I find that I get better feedback when I ask for it — instead of setting someone loose on an entire PR, I like to leave comments either in the description or inline pointing out specific areas I had trouble with, or things that leave a little nagging sensation in the back of my mind, so I can get a second opinion specifically on those things. This makes my code better, and over time, it makes me a better developer!
Code is written and reviewed by humans, and I’d love for us, as an industry, to acknowledge this in the way we ask for and give feedback. Go forth and review 💙
Top comments (0)