There’s no shortage of texts on code review out there. Still, I’d like to share with you the lessons I’ve learned about this practice, which I consider one of the most important in any software development process: code review.
If you landed here without knowing exactly what code review is, here’s a quick idea:
It’s a step in software development where one or more people on the team, other than the person who wrote the code, review a change or new feature before it’s merged into the current codebase.
The goal of this step is to catch possible mistakes or inconsistencies, and to share knowledge with other people on the team about the changes in the project’s code. These inconsistencies can range from code style to business rules or side effects of the change.
Over my career as a developer — which I consider to have truly started 4 or 5 years ago — I’ve had different experiences at the companies I’ve been through. At some, code review was just so someone else could say they skimmed the code, even without really understanding it; at another, the review was thorough, including manual tests on the branch in question. In this text I share what I consider good and bad practices in code review.
Time to review!#
A few points for the reviewer.
Leave mechanical reviewing to the machines#
Topics like code style are personal. How many discussions have you seen about spaces vs. tabs? Or single vs. double quotes? This kind of discussion shouldn’t come up on every new pull request. The team should agree on a style and be guided by it. And that style should be enforced by machines, not by humans. This can happen in several ways:
- pre-commits: before you can create a new commit, a hook checks the order of imports, whether there are lines with debug calls, excess whitespace, unused imports, etc. You can only make a commit if your code follows the rules established by the project. An example of a pre-commit config here.
- settings in your text editor: having these settings in your editor is another good way to make these practices easy to follow without much effort.
- linters in CI: linters are programs that check these styles or rules. You can put them as a step in your Continuous Integration (CI) tool. Every time someone opens a pull request, the linters run and the tool tells you whether they passed. An example of CI with linters on GitHub Actions here.
More examples can be found in my text Improving Code Quality with the Team. For these automations to be possible, the team needs to agree on a style guide and on the process for integrating new changes into the project’s code.
The most important point here is: this kind of work should be done by machines, not by humans. Instead of getting stuck thinking about spaces or tabs, the person reviewing should be free to focus on deeper aspects, like architecture and business rules.
Have empathy for the author#
Empathy is one of the most valuable skills in software development. It helps you have a better relationship with your team and to understand which problems to solve for your users. Unfortunately, it’s a hard skill to build and it takes intention and constant practice. If we don’t keep it in mind, it’s very easy to forget what it’s like to be a beginner — or, simply, a human. So, reviewer, don’t forget:
You’re reviewing code from another human, one with feelings, expectations, and flaws.
How do you practice this skill? Some advice:
- Be kind when writing. Text alone can sometimes sound cold or harsh.
- Ask what the person thinks.
- Call out the cool things they did.
- Try to explain the reasons behind a change request.
- Try to understand the decisions made. Asking won’t hurt anyone.
You can be the person nobody wants reviewing their code, or someone who makes the review process valuable. I had a person on the team whose reviews everyone wanted. Simply because she always had something interesting to add: about the impact on other parts of the code, on the business, or on the architecture. I’m a big fan of hers, actually :). This was possible thanks to technical knowledge, but above all because of empathy. She invested time in understanding the change and proposing alternatives.
No slacking off#
Many reviews go back and forth, but you can’t slack off and accept merging changes any which way. It’s unlikely someone will come back and fix it. If that’s the case, file the technical debt or bug first, and then continue the review. What matters is having shared understanding between you, the reviewer, and the author of the change.
This reminds me of a tech lead who, during a recruiting event, bragged about rewriting his team members’ changes when he didn’t like the solution. Imagine the madness! Beyond wasting a lot of time rewriting code, this person also failed to invest time in developing the people on his team. Hearing that, I used the chance to run from that company.
Understand the code you’re reviewing#
Reading someone else’s code and understanding it is a hard task that becomes easier as you get familiar with the project’s codebase as a whole. Reviewing code you’re not yet familiar with takes more effort. Start by reading the change description and the story. Try to follow the change: does it come from a field in a form? From a routine triggered when some info lands in the database? What comes next? Following that “thread” helps when it’s time to read the code itself. You’ll look at the code with the context of the problem to be solved in mind, not just the context of the technology.
When I joined Thermondo, I found a code review process where people would checkout the branch locally to see the change in action. Was that written down anywhere? No. Did anyone tell me it was required? Also no. It was a game-changer for me. Does it take more time? Absolutely. But it helped me a lot to better understand the proposed changes, fulfilling the purpose of knowledge-sharing that code review brings. I started testing the change locally and then reviewing the code. I felt I got more productive in code reviews after adopting this routine.
Review deadline#
At one of the companies I worked at, we had up to 24 hours to review pull requests. It worked well for us (at the time, a team of 7 to 10 developers). The important point of having a review deadline is to not let the code grow stale and to avoid conflicts, plus letting the person move on to another task quickly. There’s no magic number for that deadline. You should talk to your team and see what works best for you.
Time to have your code reviewed!#
A few points for the person having their code reviewed.
Is your code ready?#
Only ask for a review when your code is ready. To check if it’s ready, you can ask yourself:
Can I ship this code to production the way it is? Is it the best version?
You can’t ask for a code review when the tests are broken, the linters are failing, or a part of the feature is missing. The time of the person who will review your code is valuable. Keep that in mind.
If you opened a pull request just to see the changes or to flag that something is in progress, signal that. On GitHub you can open a pull request as draft or just add a WIP to the title (or its Portuguese equivalent TEA: work in progress? :P), or add to the description that it’s not ready to be reviewed yet.
Speaking of descriptions…#
When opening a pull request it’s important to give context. Pull requests are often used as documentation of changes. Opening a pull request with 15 modified files and no description is frustrating. So, leave a description with an explanation of what you did, the goal, a link to the story description (if any), or additional info (if it’s a bug, for example, how to reproduce it). If you’re adding a new command, for example, add instructions on how to run it. That way you help the reviewer understand your changes better, and avoid back-and-forth with questions that could’ve been answered up front.
It’s not personal#
It’s common to have back-and-forth in reviews. Sometimes I get super anxious to have a change merged, and then a flood of comments comes in. It’s part of the game. It’s important to breathe and embrace the process, which helps build not only better code but better developers. Don’t get discouraged if it happens to you; it’s normal.
Of course, every now and then we run into people with no empathy out there. I’ve cried over certain comments I got in reviews. I remember at least two occasions at different companies where this happened. At one, the person had no idea they gave a bad review. At another, not only did I give feedback to the person but people on my team who saw the pull request and the comments — even though I didn’t say anything — also gave feedback. It surprised me because I didn’t know other people would care that much. Nobody knew I cried because the person was being a jerk, but my colleagues had enough empathy to recognize that kind of behavior from the reviewer wasn’t healthy. Giving feedback in these situations is essential.
Avoid opening pull requests that are too big#
The bigger the pull request, the smaller the chance it’ll be reviewed carefully. That’s what my research institute says (kidding, I think I read that in an article but I couldn’t find it). If you’re working on a very big feature that can’t be merged straight to master, you can try:
- using feature flags; feature flags are like switches for features that you can turn on and off via code (directly with an
if/elseor using an environment variable, for example). This strategy lets you keep the code merged tomasterbut disabled until the whole feature is ready. - creating a feature branch and opening smaller pull requests against that base branch. For this you need to have organized what you have to do. It’s good to discuss the strategy with your team. That way everyone can help you think about the best way to implement a feature and keep in mind what you’re trying to do.
That’s it for today, folks#
Whether you’re the reviewer or having your code reviewed, communication is essential. Try to convey your ideas and questions clearly. Having a well-defined process with your team, and at the same time the flexibility to change, is essential for things to run well.
Hope this text was useful! Hugs and see ya next time!