Code Reviews

Let’s talk about code reviews.

I’ve seen a lot of “interesting” opinions shared recently on the merit of various styles of code reviews, and I think they all miss a very important point.

One engineer was bragging about how brutal their code reviews were, because good coding style is important. Another said they’d accept pretty much any code as long as it works.

These, and many more examples, all focus on the code itself. What about the people writing or reviewing the code? What was the purpose of the review? What did either party want to get out of that interaction?

Code reviews should be a learning opportunity for both engineers. None of us would like to have a performance review where we just got a list of our mistakes read to us, or where our manager says “eh, it doesn’t really matter as long as the work got done”. Code reviews should be no different. If they’re not helping the team, they risk becoming red-tape processes that just get blindly followed.

So what does great look like? Here are some tips I’ve picked up:

When submitting code for review:

• Ask for specific feedback. This could be as simple as “I’m relatively new to lambda functions – I’d be interested to hear your thoughts on my approach at line 250”.
• Give context for the change. Link to any supporting material (e.g. design docs), but also point out why the specific change is being made right now. If you’re patching an urgent bug in a production system, the reviewer would probably approach it differently than if you were just doing some refactoring.
• Keep reviews simple, and bound to one or two changes. It’s easier for both parties if you submit several smaller reviews than one large one.
• Have a conversation beforehand. Your MR should not contain any surprises for the reviewer – they should already be familiar with the solution you’re implementing. A code review is not a design or architecture review.

When reviewing code:

• Call out the good code as well as where you think there’s room for improvement. You’d be surprised by the impact this has.
• Ask questions as much as possible. “Do you think we should refactor this?” is better than “Please refactor this”. Using language like “Can we…” in comments shows that you’re interested in solving the problems together.
• Give examples in your comments to illustrate your feedback. These should ideally be working code that can be copied and pasted.
• Tie your comments to principles – don’t just say that something is bad, but instead explain why a different approach might be better.
• Remember the context in which the change is being made, and what the engineer asked of you – are you, the reviewer, on task?
• Be clear about what issues you need addressing, and which are optional. Don’t let perfect be the enemy of done.
• Get a second opinion if needed. Don’t assume or assert that you know better than the writer.

What have I missed? What are the characteristics of the best code reviews that you’ve seen?

Originally posted on my LinkedIn profile here