Code reviews can have a bad reputation. We’ve all heard, been witness to, or lived through horror stories where every last line is torn to shreds.
So much feedback that you have to scroll and then keep on scrolling to get to the bottom.
And the comments! So directed and personal that you would think they were meant to be personal.
The knives were brought out and we were cut up in public like a street fight.
They can make us angry, ashamed, or embarrassed. At the end of it you wonder, "what could I have done differently?"
That’s what this post is about.
What can you do differently?
How can you make reviewing our code painless so that even the biggest nitpick on your team has nothing but praise?
Let me offer you a few tips that have helped me.
The Golden Rule: Minimize the Number of Roundtrips
"Roundtrips (from teammate to reviewer back to teammate) are expensive, costing up to two working days of a sprint each time."
From: Feedback Ladders: The Code Review System We Follow at Netlify.
As much as possible reduce the number of roundtrips in the code review process.
What do I mean by roundtrips?
I mean those back and forth exchanges where the reviewer asks you why you didn’t go with a particular solution, and you have to explain why you tried what they suggested but it didn’t work.
And then the reviewer asks what problem you were solving with a particular design decision, and after your answer asks some follow up questions.
Repeat ad nauseam until by the end everyone is annoyed.
Why do I call it the golden rule?
"Do unto others as you would have them do unto you." Because I hate the back and forth when I’m reviewing someone else’s ticket. It’s what I wish others would do for code that I review, so, I’m going to do it for others.
There’s a few ways I’ve found to reduce the roundtrips.
Document ambiguous design decisions
There are times when you reach a fork in the road. You're solving a problem, and you could go with A or B (maybe even C, D, or E).
You weigh which design is the best, and you make your choice after having carefully considered each option.
Recently I got some feedback in a code review that was basically, "why didn't you go with option D?" What they didn't know was that I had that and it didn't work, so, I went with B.
What I could have done (and what I do in my better moments) is describe why I chose the solution I did, which other solutions I considered, and the ones I tried but didn't work.
You don’t always have to go into *that *much detail, but if you think there’s reason to then do it.
Most of the time I'll leave these comments in the ticket after I finish so the person reviewing has more context to help their review.
Occasionally, you'll find these comments are so valuable they should be put into the documentation to explain things like architecture and design patterns, or left as comments in the code itself.
Preemptively review your code
When you make that last commit, and push the code your job isn’t done yet.
Try to look at your code from the perspective of another person.
This might mean giving it a day for the code to sit so you can come back with fresh eyes.
Imagine it wasn't you that wrote it.
Where might there be confusion?
What issues might there be for someone else who comes to the code?
What changes do you need to make to eliminate those issues and confusion?
I’ll often go through my code following a self-review checklist I’ve put together over the years.
There's an old sales technique called anticipating objections. The idea to preempt any objections people might have by bringing them up yourself, and then addressing it ahead of time.
Same idea here. We want to anticipate the objections that could be raised against our code, and fix them ahead of time.
How do you know what those objections could be?
Once you've had enough code reviews at a company you will know the problems people have with your code. Add those issues to your code review checklist, and fix them before your code is reviewed by anyone else.
If you're getting nitpicked a ton, adopt a code linter/formatter
If people are always needling you for the formatting of your code that's a smell that your team needs some automatic code linting or a formatter like Prettier.
The code review process shouldn't concern itself with those kinds of details. It should be focused on finding bugs and design flaws.
If people are having to point out things like,
That you should or shouldn't use semicolons,
Wrap your if-statements in brackets,
Use const instead of let,
Prefer arrow functions,
etc.
that is wasting precious brainpower.
Propose that you adopt something like Prettier with the basic recommendations and be done with it.
Understand the patterns of the codebase
When I first joined my current company I was hammered pretty regularly in code reviews.
Often not for major flaws in my code, but for merely not adhering to the patterns and conventions of the codebase. Some of that was solved by the above point of using Prettier and a linter, but some of it came from understanding how the team writes code.
If your team has separate files for all components/classes don't start putting a bunch of components in a single file.
Working on a team requires a bit of give and take.
I prefer writing my code in a more functional style. Most of my team does not. There are certain ways I would write code in a personal project that I don't do at work.
This doesn't mean you don't argue for opinions, and try to get people to see your way of thinking. Or even try to introduce new patterns.
It does mean that when your team makes a decision to do things a certain way you can disagree, but then commit to doing it anyways.
Remember:
I want to wrap things up, but there’s just a few things that I’ve found helpful to keep in mind. I think they’ll be helpful for you, too.
Good code reviews require shared context
It’s not personal
It’s a learning experience
One, good code reviews require shared context
The best code reviews come from skilled engineers who have a good understanding of the problems you're trying to solve and the codebase you're working in.
If you're the only person who has any clue what you're working on, you need to create that context.
Part of that can be solved by sharing your ideas early and often.
Often what I will do is write my high-level idea for how to solve a problem, and then ask people who are familiar with the systems I'm working on for feedback.
It's a less formal RFC (Request for Comments).
If I'm working on a UI, I'll describe how I'm thinking of approaching the problem. I might provide a low-fidelity wireframe or examples from other apps that match my proposed solution. I'll tag the best UI people on my team to see what they think
If I need to mock out an API I'll write out a proposed URL structure including any parameters I might need and a potential model. I'll then tag the API people.
The goal is to see if the direction you want to go even makes sense to other people.
People will either provide helpful feedback and suggestions, or they’ll affirm you’re not crazy and it’s a good idea.
It’s just generally a good way to think through problems, too, as it forces you to think rigorously about your approach.
Also, at the end of the day when the code goes into review people aren’t going to look at your code, and be mystified with how you came up with the design you did. You worked in public from the beginning.
Two, it's not personal
This is key.
Most of the time people aren't trying to be jerks in the way they're reviewing your code.
They're just trying to make the codebase as good as they can because they want to build an excellent product.
It doesn't help that code reviews often happen over text, and most people are awful at conveying emotion in writing. So, take what's said with a grain of salt, and try not to assume malice where less insidious explanations will suffice.
We suffer more in imagination than in reality.
– Seneca
Easier said than done, Seneca!
The problem is it's hard not to take it personally. We become so tied with our code and ideas that criticism of them feels like a criticism of us as people. It feels like a questioning of our abilities.
A lot of devs don't make this any easier. They come across as rude and petulant jerks.
We have a deserved reputation for not being very compassionate and empathetic as an industry.
But we can choose to interpret others in the best way possible. We can choose to not take it personally and to view it as everyone trying to create the best possible thing we can.
And in the end when you view it that way you start to see code reviews as a collaborative process.
Like an author and an editor both working together to make a beautiful piece of writing, the code author and the code reviewer work together to make something good into something great.
Three, it's a learning experience
No matter how experienced they are, the person reviewing your code has something to teach you.
If they're a senior engineer they can teach you about some method you didn't know existed that can solve your problems in the half the lines of code that it took you, or some other interesting approach.
If they're a junior engineer they can ask you the kinds of questions that reveal you don't understand things quite as well as you thought because otherwise, you would be able to explain why you went with a particular solution.
Whatever happens in our code reviews we should strive to learn and grow from them.
Earlier in my career, I started trying to learn from every piece of feedback, and to eliminate things people knocked me multiple times for in reviews.
We can all do that.
The End State
I can only speak from my own experience, but, as I've followed each of these practices over the last few years, code reviews have lost their sense of dread. I used to hate moving my tickets from being in development to ready for peer review.
No longer.
Reviews have become an opportunity for me to learn and improve.
As I've made my code less painful for others to review something remarkable has happened:
The review itself has lost its pain for me.
Top comments (0)