Tips For Better Code Reviews
Heads Up!
This article is several years old now, and much has happened since then, so please keep that in mind while reading it.
Code reviews are supposed to be collaborative and not a one-way process as is often seen – a senior dev reviews the code and where / why it’s wrong. I’d love to share my tips for running better code reviews on both work and open source projects.
The missing step
Counter to common practice, the code review process should start even before a single line of code has been written. It would be a terrible shame to put in hours of work only for the PR to be rejected because it doesn’t meet requirements, or the code isn’t a suitable fit for the project.
Grab a colleague to appraise your proposed approach before you begin. Discussing the task and the solution can help you ensure you’ve understood correctly, that you’ve considered any relevant edge cases, or haven’t over engineered something. This is a great opportunity to discuss any challenges you think you might come up against and how you might tackle them.
Another key pre-code step is to identify any patterns being followed in the project and ensure your solution matches where possible. If you call some, or change the whole project to use tabs instead of spaces, chances are your PR will be rejected.
This advice is arguably just as (if not more) relevant for open source projects too. Maintainers don’t bite and will be likely excited to hear you want to give back – raise an issue or reach out to a maintainer before you get started to make sure it’s a change they’ll accept and any pointers they can give.
I am not a robot
The bigger the PR, the harder it is for a reviewer to process. Humans have limited attention spans and time in the day, so keeping your PR easy to digest will get far better results.
Adding a clear and concise description to the PR overview can be good. Consider what a reviewer might need to know to acutely understand your changes – a list of requirements, instructions for how to test, and maybe some proof it’s working in the form of passing tests or a screenshot?
As a rule of thumb:
- If you are introducing new functionality, explain what the feature is supposed to do
- If you are changing existing behaviour, explain how it worked previously, how it works now, and why the changes
- If you are refactoring logic, explain why this is in the interest of the project - maintainability, performance, etc
For long winded PRs with a lot of changes I really like when I can step through the process. Committing your changes at regular intervals with clear and detailed messages can really help with the review, perhaps even clarifying your intent with the code in case anything was unclear and preventing the need for back-and-forth on the review comments.
Don’t worry about having a lot of noise in the repo, they can be squashed into a single commit or cleaned up later. Github has these options already built in when merging a PR!
It goes without saying that “Changes”, “Oops… broke something” or “FFS!” are terrible commit messages because it’s not at all clear what was changed. Guaranteed we’re all probably guilty of it once in a while! Probably the most common message I have seen would be something like “Changing the search logic”. This is certainly progress, but lacks key information of exactly what was changed and why. Here's the same commit message but now with a lot more information “Introducing facets into the search logic so results can be filtered by category”. It didn’t take too many more words and yet the outcome of the change is instantly obvious.
Tone of voice
Okay, maybe the code in front of you really is the worst you’ve ever seen… We've all seen some dumpster fires, but it is important to keep code reviews a constructive process. It can be a daunting experience for any developer to have their work “judged”, so treat it with the same level of attention and care as you would wish in return.
Which of the following comments would you rather receive?
What is the difference between X helper method and this new method you have created?
Versus:
This method duplicates X helper method. Please remove it.
Of course everyone will have their preference here. Personally, I prefer #1. Rather than making an assertion the feedback is phrased as questions to instigate a discussion and evoke cooperation. Whereas #2 feels like a direct order - maybe the reviewer has good reasons for their comment, but on the face of it it comes across as confrontational and is slightly lacking in empathy with the dev.
The age-old internet challenge of getting across tone and intent is ever present in pull requests. In the right time and place you may find GIFs, emojis, memes are your friend. Eh?! Yes, really! It’s a damning statement of humanity that these have become a social crutch, but there are occasions where they can add value.
Maybe you don’t want to be sending your favourite cat memes to a client reviewing your code, but I have been known to drop the odd smiley face into some feedback to lighten it up. 😊
Of course giving feedback works both ways. If you really like the work someone has done that should be highlighted too! Drop your favourite “You Rock!” GIF in there and tell them what a great job they did – chances are it will make their day and they’ll be even more receptive to your feedback in future.
Did you know: You’re not limited to just the emojis that Github offers, press Control + Command + Space on a Mac or Control + . (period) on Windows to reveal a full suite of emojis.
Shake it up
Keeping engagement levels up on a PR is essential for its survival, no activity for ~24 hours and the chances of it being merged quickly rapidly start to disappear. For open source projects you may need to be a little more patient, please don’t forget maintainers are probably generously giving their free time. ❤️ There can be any number of reasons why a review goes quiet, devs get sidetracked by their next task or put large daunting PRs to the bottom of their list.
I’m a big proponent of using video calls to replace 95% of the code review process – have the developer talk through the changes they made and demo the end result, asking questions as you go. Communicating feedback in this way is far more human, it prevents misunderstandings, and encourages discussion. Getting a demo helps verify the code works and also makes it easier to review aspects of the project that would be otherwise tough, such as the user experience, plus helps you to understand the context behind the code you’re reviewing.
Video calls enable a review to have passive and active participants. Inviting a variety of team members along to listen in is a nice way to share knowledge.
Learning to verbally communicate the changes you have made is a valuable skill when it comes to. You’d be surprised just how many issues you yourself may spot simply by talking through it aloud.
Callum Whyte
Callum is on Twitter as @callumbwhyte