I can't say I know everything about code reviews, but I have learned a thing or two doing them at a pretty large company over the last half-decade with a variety of peers. The biggest takeaways, if you find nothing more, that I encourage you to apply to your code reviews:
- ask questions
- assume the best of others
- be honest, be respectful
Asking questions is a great way to acknowledge & explore the solution/change that an author is presenting in the code review. What's unfortunate is that I'm (anecdotally) seeing less conversation on code reviews in the past few years in favor of moving quickly and keeping the velocity up up up. Questions that could have been asked during code review – ones that might even ask “the obvious” – can eliminate frustrating late night conversations had after being paged! Avoiding a misstep isn't the only benefit to asking questions.
The biggest benefit to asking questions – almost any question during code review – is that not only do the participants get their questions answered, but it also disperses the knowledge being discussed. Think “ideal university lecturer with lots of engagement”: everyone wins (if they're paying attention).
Reflecting on this a bit right now, I think we're beginning to focus too heavily on the “do it now” part of software development to even consider encouraging questions that folks new to a team, community, or company might ask. I'm biased towards wanting to, myself, understand and to share what I know with others – I enjoy teaching and helping others grow – so I'm nearly always inclined to keep folks on the same page, even at the cost of velocity. Why? I like folks on the same page because then that investment pays off in dividends in being able to take on more and more complex projects. It builds a culture of understanding and mentorship between engineers. Most of all, investing time to establish a shared understanding means that any/all of the engineers can tackle challenges with skill, expertise, and curiosity.
assume the best of others
You know who (probably|most-likely|absolutely) isn't intentionally throwing away an error that they should be handling? That person on the other end of the code review.
Personally speaking, I've seen a few different faces on the receiving end of code reviews: pedantry, unhealthy/stubborn skepticism, blatant disregard for shared goals, ambivalence, and total engagement. They all have their drawbacks and not all of them can be constructively communicated with.
Can you imagine folks thinking the worst of you, your code, and decisions that led you there? If you've been in the industry and seen this: I'm sorry, that sucks for you and sucks for the folks involved (whether they know it or not, its their loss as well). When it happens in a review, you can feel it. To do anything other than assume the best is to allow communication to be suspect to negative biases, inter-personal challenges, or – more innocently – plain & simple disengagement.
I implore every reviewer to assume the best of the code review's author & their intentions. Point out flaws, but do not assume anything about those flaws without starting a conversation to clear up a misunderstanding or resolve a latent bug!
Existing hang ups are difficult or impossible to avoid in some cases, and even then both the author and reviewers can balance out if they're all (at a minimum) trying to be positive and assume the best of the author. An entire team working to keep that “good energy” going is bound to pour that back into themselves & new engineers. In the same way that companies like Amazon have found success modeling their strategy around a virtuous cycle, so can code reviews invest into their participants by leaving every person free (free of the burden of assumptions) to ask questions, to be wrong (and to be right!), and teach each other in the process.
be honest, be respectful
Honestly, I have no credentials to speak to the psychology of this all. Take this with a grain of salt.
In my experience, balancing both honesty and respect in your words is the hardest part to practice and use in the code review.
To be honest with yourself (in that you speak your mind, your thoughts, and voice your intuition as constructive feedback in the review) might mean writing Typing
this function's error handling isn't great (which you might honestly think and judge the code as such) is still a ways from
I think we need to handle a few more edge cases in this function. In this case .... Granted: yes, there are more words. That's not a bad thing, but it can be if you wind up saying nothing. The tradeoff you are making is to trade time for consciously written words – the return is great: you're not starting an argument about the code and, ideally, there's no mistaking your words as making a slight against the author.
Writing positive criticisms is a wordsmithing artform – it'll take practice and time to get the feel for. I recommend adopting the principals (and framework) of Nonviolent Communication – it's largely a superset of the above points in that its goals are specifically intended to foster empathy and explicitly not to avoid disagreements. The psychology behind the concept, to me anyway, seems to be well in line with code review! You need to be able to disagree, but also have constructive, productive, and collaborative conversation immediately follow. For those still not sold, here's an excerpt from the above linked Wikipedia article on Nonviolent Communication (NVC):
It is not a technique to end disagreements, but rather a method designed to increase empathy and improve the quality of life of those who utilize the method and the people around them.
I've personally seen conversation thrive in a setting of NVC, much more than one without (or with a bias towards terse corrections) – I can attest that not only does it improve (my, an engineer using NVC) quality of life but also the code I and my coworkers are working on. The technique is certainly not for everyone and every situation (or every code review for that matter) but it is a helpful tool to kick things off.
Regardless of what communication tools and patterns you reach for, I encourage you to remain honest – freely providing feedback and your thoughts – while respecting others with your delivery. Internalizing communication habits take time and will be a conscious struggle until its not – but the effort is worth it. For everyone.
So, this is a bit of a rant, but I needed to get my own thoughts collected. In the last 2 years, I've switched teams, had engineers come and go on the team. In one month I even saw a NOTABLE improvement to the team's code review habits and feedback when a frequently argumentative engineer left the team.
Not to toot my own horn, but the folks on my current team have privately messaged me and thanking me for cracking the door on code reviews for them. I've had discussions and brief exchanges with folks that largely find the same: having positive interactions that leave room for questions, for discussion, that also do not belittle (whether by strongarming or words) is a damn sight better than one with no discussion and certainly better than one with a one sided or negative review.
I'm convinced that engineers teach other engineers how to talk. So here's me throwing my part in for that effort!