Intro
After doing many code reviews, I’m sharing in this post my advice about how to perform them well.
- Why are they important?
- What to look for?
- How to write comments if you consider that something needs to be changed or can be improved?
Before starting, when I refer to CL, I refer to a “changelist” (aka: a pull request).
Let’s go 🚀
Why are code reviews so important?
Code reviews are extremely important to ensure that the potential changes to the codebase are maintaining or improving the quality of the system. They are also great to review that the change is doing what is supposed to do. Last but not least, they are an amazing tool to share knowledge between engineers, and to trigger valuable discussions.
Some tips…
Some advices I can give you based on my learnings & own experiences:
- Make sure it’s clear for everyone in the team, that reviews are as important as writing new code (and they deserve the same level of attention).
- If you are about to review a CL, make sure you can dedicate the proper time to do a proper and thoroughly review (my piece of advice: it is better not to do a review if you can’t focus properly on it, than to do it in a rush).
What to look for in a code review?
I guess everyone has a different way of reviewing code. But I think that the main things to look for are:
- Checking that the title & the description of the CL are clearly stating the problem that it’s trying to be solved (and briefly, how it’s being solved)
- Check the CL has proper unit tests (or integration tests & E2E tests when applicable), and that they are covering edge cases.
- Check that the CL is solving the problem that is supposed to be solved.
- Check if the change is being added in the right codebase. Does the code really belong there?
- Is the change compatible with the long term vision of the team?
- Is the change aligned with the overall architecture of the system?
- Can you find any bugs in the code? (important: this is not the main goal of a code review, and we shouldn’t rely on code reviews by any means for this – that’s what automated tests are designed for).
- Throughly check the implementation (line by line), trying to spot improvements in terms of:
- Design (eg: Is the solution well designed? Can the solution be designed in a better way? Maybe a design pattern could be useful to solve something differently? Are SOLID principles, DRY, YAGNI & other best practices being followed?).
- Can we make the code simpler?
- Can we make the code more readable?
- Can we reduce cyclomatic complexity?
- Clean code (eg: Is the code clean? Are the names of the variables/functions/etc good? Is the code in the right place?).
- Efficiency (eg: Is there a way to make the solution more efficient in terms of time complexity — space complexity?).
- Security (eg: Is the code introducing any vulnerability? Is it following security best practices?).
- Documentation (eg: Is the documentation being updated/added?).
- Error handling (eg: Are errors being handled properly?).
- Observability (eg: Is the change observable? Do we have the appropriate logs, metrics, etc…, to monitor the change?)
How to get better and faster reviews? (for code authors)
- Keep your CLs small (I’ve learned from my own experience, that raising big CLs with many LoC being changed are not only harder to get reviewed properly, but they also take much more time to get reviewed). Really, nobody wants to review a huge CL. This kind of CLs are also more prone to be skipped. Interesting read.
- Keep your CLs focused on doing only 1 thing.
- Self review your CL before sharing it, not once, but twice!
- Try to be available to answer any question that might come up during the review.
- Last but not least, ping people if they are not reviewing your CL after some time (eg: more than a day). It might be an indication that your CL is not clear enough, or is not broken down efficiently into smaller ones, etc… I suggest asking for feedback to the reviewers as well, specially in those cases.
How to write good comments? (as a reviewer)
The whole point of a code review is to find opportunities of improvement, and suggest them to the CL author.
When writing a comment I really recommend you to:
- Always be kind & communicate the suggestions in a respectful manner. Be mindful that the author probably spent lot of time on it.
- Give clear explanations of why you are suggesting the change, and support your comments with facts whenever possible.
- If the suggestion is not really a strong opinion, but more of an optional change (or a matter of personal taste), make that clear to the author.
- If you add a comment, try to check later if the author applied the change, or replied to your suggestion. Be mindful of not “blocking” the CL since otherwise it will slow down the team.
- Keep your ego aside. Always be humble when you write a review, and do it just with the goal of improving the quality of the overall codebase, or helping the author to learn new things.
- Always have in mind that there are multiple ways of doing things (and sometimes all of them are valid). Sometimes it’s clear enough that one solution might be better than the other/s, but that’s not always the case. Only ask for a change if you are really convinced that your alternative is better.
Final words
A nice codebase can turn into spaghetti code really quickly. The only way of avoiding that, is taking code reviews seriously & always having an eye on checking that the quality is being maintained or whenever possible: improved.
I really recommend reading this guide from Google, that dives much deeper into this topic and gives really valuable recommendations.
I hope to have contributed at least a little bit to your journey as an engineer 🙂
Thanks for reading!
Disclaimer: This post represents just my thoughts and my personal opinion about this subject. I’m not talking on behalf of any company I work, or I’ve worked for, nor giving real examples from neither of them.
Leave a Reply