Trey Kane Logo

Trey Kane

Code Review for Everyone!

February 22, 2024

Back to home

I like many people find myself often having to provide code reviews and offer feedback to my coworkers and friends.  But like anyone else I often wonder if I’m doing it well, and can I do this better?

I set out to find some advice…

I myself have been doing day-to-day code reviews for about 6 years now. The benefits are clear: your work gets checked for errors, someone else gets to learn from your solution, and collaboration helps improve organizational cohesion.

A good code review will cover the organizational or project best practices, and check that everyones work if following similar style. Pointing out obvious improvements, like hard to understand code, bad naming, test or debugging left in place, ensuring code has unit tests, or even identifying edge cases that were overlooked.

But how can we start to do this better?  How can we make small changes to do better code reviews everyday?

  • What should we look at when reviewing code?
    • Obvious things to look at are spacing, style, naming, common bugs and mistakes.
    • Do you best not to scan the document, but attempt to look at every written line… Ignore files created by robots.
    • There are things that are often harder to identify with out more contextual knowledge, such as edge cases that may be overlooked, or the state of unit testing on a given project.
    • Then there are things that take time to identify. These things don’t easily fall into a bucket or category, and often require the full context of a piece of work to identify. This would involve the reviewer pulling and loading the feature branch to test the change in their local environment. While this is a very good practice for a large set of changes, it’s not always practical. This is also where we think about the broader impacts of a given change
    • This is not to say every code review needs the most granular level of testing, but its something to keep in mind before you hit the Approve button and if you have an questions about possible impacts or functionality, before you hit approve is the time to check that.
    • But we should take the time to understand the context of the work, and what our goals are, and what the outcomes should look like.
  • How to shape the tone of your code review
    • Unimportant comments, unless other changes are included in the file these things can be spacing, a single properties being unalphabetized, brackets being left on the same line. It may also be worth noting these things, but offer them as optional feedback. For the most part, listing tools help us resolve these problems before they ever make it into the PR. But be mindful of the time sink we create when we pick apart PRs to this level.
    • Avoid Opinionated Language
    • Ask Open Ended Questions and offer potential solutions or alternatives, but avoid insisting those solutions are the only way.
    • Do your best to convey a positive tone
    • Exercise empathy
    • Offer positive feedback when you see something you like!
  • Code Review Across teams and timezones
    • Perform code reviews during overlapping work hours.
    • When submitting code reviews, be mindful of the working hours of the reviewer.
    • Reviewers should strive to keep the time between request and review minimal, so if you received code for review overnight, try your best to address it first thing in the morning — assuming this is also overlapping working hours.
    • Always strive to unblock your teammates and partner teams, code reviews are an easy way to help achieve that.
  • Approving Changes
    • Don’t approve changes with open-ended questions
    • Make clear which pieces of feedback are blocking, and which are not
    • You can approve a PR with questions, but make the conscious decision that these questions should not hold up the work and are optional follow up for the engineer. 9/10 they will follow up on your questions before merge, because we work with good people.
    • Be flexible, your solutions are not the only solutions, and what exactly the best solution is can be relative to its use case.
  • Requesting Changes
    • Request changes when anything you have identified has an open or unanswered question.
    • Request changes when something is obviously wrong.
    • Us “I” messages, as formulating these things from your point of view will immediately remove an accusatory tone.
    • Requesting changes can be a tricky thing for some people. Many don’t do it lightly as if we’re exercising empathy we know someone put a lot of time, effort and thought into this code.
    • Sometimes changes are better made in a followup PR, but these changes should be immediately followed up, if let to go stale it’s likely the changes will never happen due to other priorities.
    • One of your goals as a reviewer should be to always unblock the engineer or enable them to take some sort of action quickly, without sacrificing code health.
  • Have a conversation
    • Talking to people is a sure way to make things better and make sure messages are communicated clearly.
    • When a conversation starts to get confusing or the exchange on the PR starts to become lengthy, have a conversation about the code. Sometimes too many comments might indicate a misunderstanding somewhere that should be cleared up.
    • If you don’t understand a change but have been asked to review it… Ask to be walked through the code, and a quick demo! This should take 15 minutes or less and will help everyone to feel better about the work completed.
    • Everyone involved is human, exercise empathy and talk about it, everyone wants to do great work!
  • Learn something
    • I’m a firm believer that if you’re going to take the time to do something, you should do your best to learn something from that experience , not only that actively consider what you may have learned. You have to know what you know to be able to know you know how to do something.

Support for code reviews will vary with every organization and project team, but it’s not something that should be taken as a trivial ceremony.  Good code reviews and code review practices can greatly increase the effectiveness of an engineering team. I would suggest always being an advocate for code reviews on your project, no code should make it to production without a code review.

This is not something that any of us will get better at overnight. We’re all busy, and have different demands being made of our time constantly. But being mindful of what we’re looking at and what we’re looking for when performing a code review will help us get incrementally better all the time.

Phillip Hauer who I borrowed a lot of these ideas from had a really cool image in his 2018 blog post “Code Reviews for Humans” that I thought summed up everything nicely. I also highly recommend following up with his blog post, its a wonderful read!