Note: I wrote this post for an internal team blog, but thought it was worth sharing more widely.
Part of getting to good code reviews is some up front discussion about trade-offs and implications for bigger architectural changes. I think of code review as when “my” code becomes “our” code – for architecture, those conversations need to start earlier. We all live with it, decisions have consequences beyond the project we are currently working on, and it has a huge impact on our ability to execute over time.
Some things to think about when giving feedback:
Ask questions. If it’s not clear to you 1) it’s probably not just you 2) it’s still worth clarifying.
Think further out. How does the proposal affect things in 6 months? 12? We might choose a shorter term option, but we should make a mindful choice.
Consider the effort vs the impact. I think this is a really important skill as an engineer or designer that I expect everyone on the team to have some sense of. We hire experienced people who we can trust to be autonomous, and I think this skill is pretty critical to that level of trust and autonomy.
Don’t nitpick. Small details are distracting. Big picture feedback is more important. If something is a nitpick, clearly mark it as such – and then consider whether it’s worth nitpicking at all. When that nitpicking can be automated, let’s do that. It’s fine to be reminded by a script. It’s not a good use of anyone’s time to be reminded by another human.
Style guides. It’s easy to have opinion on style, but consistency is more impactful than the details of it. The purpose of a style guide is to never discuss it again. Every other discussion should focus on the substance of what is being proposed, not style.
We need to be less afraid to give people feedback in general. Technical feedback is a great place to start since 1) that is our focus 2) it is not personal. You can question and critique someone’s idea or proposal without attacking them as a person, and being able to have our ideas and proposals critiqued without taking it personally is important for any professional. We don’t have to agree with each other on everything in order to treat each other with consideration and respect.
Some things to think about when asking for feedback:
Put it in the right place. Small changes are best discussed in PRs. Bigger changes are for the internal blog (but as an OSS project should be in the README architecture document once decided).
Be clear about the problem you’re addressing. This is really helpful context for people to understand where you’re coming from.
Explain why it’s important. Make a case for the impact, and why now.
Talk about what you’ve considered. What are the alternatives and their tradeoffs? Why do you think this is the best option?
Think about the kind of feedback you want. What would be most helpful? What might other people have more knowledge of?
Be clear on next steps. When do you need to make a decision? What do you expect to happen next? Who do you need to agree / help?
The goal of these discussions is to define a path forward, and they should end with a specific decision which we then act on. Non-decision decisions* are a common dysfunction that I strongly prefer we avoid. Have some back and forth, switch to a call if necessary, but at the end of the thread there should be a decision and some next actions. Review the feedback, answer the questions, and then based on that, circle back, summarize, and state the decision.
If the thread has been productive, and people feel heard, this might still feel scary but at this point we have all explained our point of view, and should be willing to accept the outcome.
* Non-decision decisions: where a decision is made by not making a decision.