Let's talk about code reviews
In this in-depth article, I'm sharing my knowledge and thoughts about the code review process, no matter at which stage of the code review journey you currently are.
Do you review your code? Are you forced to do it? Do you feel like it's working well? Or maybe it feels like like you’re wasting time?
If you are already doing code reviews, you might not think a lot about it. But what if it's a missed opportunity? What if you could get even more out of your Pull Requests?
Note: In this article, I might use pull request (PR) and code review interchangeably. To be precise, a pull request is a thing you create in some tool, containing some description, diff of changes and can be approved/declined, and code review is the process of reading the code. For the sake of this article, I don't feel like these two need to be differentiated.
Code review goals
Let's start by answering "Why should we even do code reviews on our team?". I think it's not that straightforward to answer, there's no single answer to this question. As usual, it depends and varies from team to team, but here are some things I find beneficial from reviewing the code:
Knowledge sharing
This is the biggest thing in my opinion. The ability to easily transfer knowledge between team members. And it does not even matter whether they are seniors, juniors, or any mix of these. Knowledge sharing is always happening and is always beneficial. In my case, it happened multiple times that some innocent and small comment on PR started a bigger discussion, sometimes even one that I could call a philosophical one, like the int vs var/let. And I think it's a good thing, since such discussions, if are moderated properly (so we're exchanging our opinions and giving arguments, instead of trying to push as hard as possible onto our favourite method), can lead to the growth of all members within the team. Code reviews are giving opportunities to such discussions, they act as conversation starters. After all, it's not a regular practice (at least not in my case) to approach my coworkers and ask them random questions, like "Hey, how do you usually decide that the function is already too long?" or "Have you learned something new recently you could share with me?".
Quality improvement
This is another big benefit of code reviews. You always have at least a second pair of eyes, at minimum skimming through the code and checking if you made some of the most obvious mistakes. Depending on the type of change and reviewer, you might get a more detailed review or just a quick look at the code. While I prefer more detailed reviews, they also take more time and energy, so I can understand it's not always possible.
When I was at the university, I had classes on waterfall methodology. In waterfall, you have multiple separate stages (requirements / design / implementation / ..) and identifying a bug in each next stage increases the cost of fixing a problem by 10x. I'm not sure how true it is, but I believe that something similar happens on pull requests - when you catch a bug in PR instead of in a test environment, you save yourself some time. If we compare it to catching a bug in a production environment, the cost is usually even higher. It does not matter whether we're talking about business logic bugs, or technical ones (like memory leaks), it's always better not to introduce them.
Tests
Depending on your policy on testing, PR is a good point to ask "Is it possible to add some tests?". I also sometimes sit down and try to understand what the tests are testing and whether there are some problems in the tests themselves, or in test coverage.
Keeping up with changes
If the project is big enough, everyone can't know what's happening in every module. Code reviews are a cheap way of keeping either everyone or some subset of developers up to date with the codebase, without investing too much time into it. They don't have to do the implementation, all they need to do is to spend significantly less time from time to time checking out the PRs.
What should not be a part of a code review
There are also some things that PRs and code reviews are not about. These are things like:
Code style, formatting, newlines, etc.
Build checks (reviewer making sure that app builds)
Test checks (reviewer running the tests on their machine)
These things can be easily automated. Once the code does not compile or tests are not passing, it should be impossible to merge the code into some sort of main branch and there's no need to waste the reviewer's time to check this.
The same thing goes with formatting - as a team, you should agree on a set of rules, set up automatic linter and either enforce linting or fail the build on failed linter check.
Code review is about code, not about the author's ego
As a code review author, remember: it's not about you. It's about the code. What I mean by that is that when someone points out that you've introduced a bug, there's a problem with your architecture, or the code is hard to understand, try not to take it personally. There's an API rule which I try to follow - Assume Positive Intent. It simply means that you should assume the person on the other side has positive intent. This is especially true in the remote world - it's hard to pass the emotions through the review comments. Whenever there's a space for uncertainty whether the person has good intent or is just malicious, always assume the good intent. It's a really small thing, but can massively improve the atmosphere at work. From my experience in almost all cases, people have good intents at work and even when they're wrong or are defending something obviously incorrect, it's possible to calmly talk it through. And most importantly - it always might be you who's wrong. This way you avoid being a jerk.
Different approaches
I've been working in multiple teams, and each team had way different approach to code reviews. In some teams we had no code review and I think this approach is wrong and no matter what. The code review should be a part of every development team.
I like the 'deep dive' approach to PRs. So spending a long time reviewing the code and trying to understand all the "what's and why's" of the reviewed code. But there's also a different approach which I also think is fine.
A "shallow review". It takes a small amount of time and it's just meant to catch some bigger or more obvious problems. The reviewer skims over the code and stops only after something catches his eye. I would recommend this technique for experienced teams where you expect that you can trust all the devs that they know what they're doing and they're taking responsibility for their code - without taking responsibility it's a recipe for tragedy.
Code review comments
Writing good code review comments is an art. What I've found working the best is 'ask don't tell'. So instead of writing 'Add some unit tests', ask: 'Is it possible to add some unit tests?'. This way you open the gates for discussion. Because it's not always that the other person does not want to do it - sometimes there's a good reason why it wasn't done in the first place. I cannot count how many times I've approached the code with the attitude 'Why the hell is it done in such a weird way? Let me fix it real quick' only to realize an hour or three later why and back off.
Second thing - be polite. It might be the most important point. If you write a comment 'Why are you using library X instead of Y, are you stupid?', I can almost guarantee you that the other person will either do everything not to do your way or will back off without any further discussion. This is of course a bit exaggerated example, but I've seen cases similarly extreme to this one.
Let's also try to approach the problem from the other side. What if you've received a comment ending with '...are you stupid?' How can you approach it? I believe there is only one good solution - to talk it through. First I would maybe go to another coworker and ask 'Do you also think that this comment is not appropriate?', to gather a second opinion. When I'm in emotions I don't always think straight so it's good to have someone to ensure you're not overreacting. And then when I get the confirmation, I'd go and talk with someone. Usually it would be a team leader or manager, but if you're feeling brave, you can go to the person directly. If the person is not just 'mean by definition' but is having a bad day, I can guarantee you that they will appreciate your openness a lot. And if they won't, you don't want to work with them and the problem is way deeper.
Remember to NEVER respond in anger. If you feel like one of the comments touched you deeply, go for a coffee or a quick walk. Go to the nearest window and look at the nature for 3 minutes. Go to another coworker to talk about their kids. Do everything but respond right away. These 3 minutes can save the atmosphere and give you bonus points for your ability to handle difficult situations. Another outcome might be that when you come back with a cookie and coffee, you'll realize that it's not that bad, the reviewer was kind of right and maybe you're the one having a bad day. You won't become a drama queen.
Another idea for review comments is adding comment size or relevancy to it. One of the techniques is the 't-shirt size' thingy. At the very beginning of each comment, you put a t-shirt size. Let it be:
[s]: you can freely ignore this comment
[m]: it's something you should either fix or we should discuss
[l]: I'm convinced that there's a bug here
or a lighter version of it, just marking 'irrelevant' comments as nitpick with 'nit'. The difference between a regular comment and a nit comment is that nit comment is usually about the thing where you either want to slightly suggest some change but it's so small that it does not really matter if it stays or not. It might be also something that is a personal preference which you know you should not enforce. It can also be a 'fyi' comment - when a thing is implemented in a correct way, but there's a new feature of the language that allows to simplify it.
Have a call besides comments
When you have a non-trivial review, it sometimes means that you've received or created multiple comments. Some might be big and important, and others you might be unsure of.
How you could approach it as a reviewer is the following: first of all sit down and try to understand the code and changes by yourself. Go file by file and review it the best you can. If you don't understand something, write a comment that you don't understand or you're unsure of something. After you've done the whole review, grab the author on a call. From my experience, they tend to be long calls, since you're probably discussing potential problems and solutions. On the call you can walk comment by comment and clear all unknowns, ultimately saving time, and also bringing both of you onto the same page. Sometimes people are willing to just 'do what the reviewer wrote without too much thinking' instead of trying to understand. This way you can also explain the 'why's' behind the comments and your thinking.
The same goes when you're the author of PR. If you feel like you don't fully understand what reviewer meant by the lengthy and complex comment (or maybe a small and simple one?), just ask them to hop on a call. In the long term it tends to save a lot of time.
What should I look for at the code review
It also varies a lot from team to team. But usually, some core things remain the same among different teams. These are:
Is the business logic implemented according to requirements?
Are there any technical problems with implementation? Could it be improved / optimized / rewritten according to team standards?
Typos
Tests - are there any tests? Is the coverage enough? Are the tests on a correct level? Are there any cases not covered by tests?
Documentation - are the changes documented properly?
Overall compliance with team agreements
If you have agreed to have some steps as part of code review (for example, you should manually test each PR before merging) you could try to make a template which will either automatically or manually be pasted into the PR description so that you can publicly tick off points you already did and every reviewer can check it out without explicitly asking.
Review in browser vs local IDE
Should you review the code in the browser or should you use your local IDE? I've used multiple online platforms: GitHub, Bitbucket, Upsource, Azure DevOps. They all differ by a bit but have one thing in common - they are not IDE. They all don't have the capabilities of your local dev environment. You cannot run tests (okay, you have the CI pipeline, but you get my point). You can sometimes go to the definition of function or variable, but not always. You don't have the full capabilities of mouse-over something to check its type and usages. It's closer to Notepad than IDE.
But on the other hand, they are all always one click away. You don't have to checkout the branch locally. You don't have to stash your changes. You can click some URL in a browser and that's it, you're ready to review. Also, they sometimes have some nice features like marking which commit / file / batch of changes you've already reviewed and it's saved for later.
The decision to do a review online or locally might seem to be minor and unimportant, but I think that it's significant. Maybe not crucial, but it might affect the way you write code. For example, let/var vs type name - if you are in your local IDE, you can look it up by hovering the mouse over it. In the browser, it's usually not available. In such case, you should write full type names for readability.
Another thing is the context. When I review the code, in bigger PRs I like to checkout the code locally and open it in IDE. Then I start reviewig the code in some file and jump around between functions trying to understand the flow. So instead of going top-to-bottom by filename or folder name, I would be guided by the flow of logic. This way I'm reviewing not only the code as is, but I'm also able to build a bigger picture about the changes and business logic inside. I also have easy access to the rest of the codebase, so I can quickly look up code that is not part of reviewed changes.
So, which one is better? I don't know. I think that it depends on the team you're in. I am comfortable with doing both.
Code review size
How big my code review should be? It also depends. If you have to wait a week for the review, I'd say 'as big as possible'. If you can get your code reviewed right away, I think that 'as small as possible' might be the answer. But I don't have a definite answer here. Smaller PRs encourage reviewers to do the review faster. I am also aware that unfortunately there are a lot of people who will become over-nitpicky with small PRs and when they get a big PR, they will skim over it, approving the changes without actually reviewing.
Another factor is the completeness of the feature you're implementing. Should you split PR into (code) and (tests) PRs? Probably no. But depending on the exact details of a feature you're implementing, you could try splitting it into 'vertical slices' - implement a part of a feature that is isolated and a complete piece of code, make a PR, merge, and start working on another small piece. You need to try out different sizes and see what works best.
When should I review PRs
No later than half a day from any point of the day. Code review is often a blocking process, meaning that if you don't unblock your peers regularly, they get to the point where they have 7 PRs open and they are getting lost in it, not to mention reviewing this all. If you don't like to be interrupted during your day, you have at minimum two points when you're already 'not in the zone'. The first one is when you've just started working in the morning. The second one is after your lunch. Reviewing code is working towards the whole team's goal so you shouldn't wait with review longer than necessary. I try to do the reviews as often as possible. If I'm busy with some critical tasks and someone asks me for a review, then I ask them if the review can wait. It should not happen too often, but in reality it does sometimes. Then they at least know that they'll have to wait. They can find someone else to review their code, wait for me to finish off what I'm doing currently or we could discuss re-prioritization. I'm talking about bigger reviews (let's say 30+ files, 100 lines changed in each) requiring even up to a couple of hours to review. Tiny PRs have a smaller risk of having this problem.
When should I create PR
Again, this depends on multiple factors. The first one might be CI pipelines and tests. Are there any slow-running tests that can be run only in the pipeline? Or the ones being too slow to run locally? If the answer is yes, then it might be beneficial to create PR right away if you have a pipeline tied to PR creation.
Another thing to consider is catching big problems early. The earlier in implementation you find that the architecture decision you've made is bad, the less time you've wasted working on a code that will get deleted anyway.
If none of the above applies, I'd say that you should probably create PR once the feature is fully implemented, or at least 90% implemented (so that you have a chance to implement the missing test while someone reviews the code, or improve insignificant things alongside with PR changes from comments.
Pair programming - can we skip the review?
What if you pair-programmed a feature? Can it be treated as already reviewed?
The answer I give to this question is "probably yes". It of course depends on how cautious you are while coding. In some cases, a quick self-review would be beneficial.
Self-review
I like self-reviewing the code before I hand it over to my teammates. It has two main benefits:
You gain credibility in your team - once you create a PR, there's a high level of trust that your code works. It has advantages in situations like yearly reviews where you can bring it up.
You can catch a lot of things by yourself, without even bringing another person to the table. You save everyone's time, and once reviewers see that you're not wasting their time submitting half-baked reviews, they at least should appreciate it and make the review faster.
So, how to do it? In the same way as you would do any other review. Grab your favourite tool, create a draft PR (or some equivalent of that) and go file by file, looking for potential problems, leftover code and typos.
I don't think that self-review can fully replace second pair of eyes looking at your code, but it's a good habit.
Self-review has also a second benefit. You don't need another person to do it. What it means is that it's a perfect tool for some 'emergency' situations when you don't have a reviewer. This technique saved me some time and stress when I was doing quick hotfixes outside of regular business hours when no one else was working. To do a self-review, create a PR as you would normally do, and go for a 5-minute break. This way you are leaving 'the zone' and when you get back to your computer to make a review, you are as close to being an independent reviewer as possible.
Should author of PR fix code outside of theirs changes?
It often happens that there's a piece of code that needs to be refactored but is not a part of current changes. These are either some TODOs, code containing bugs, or the whole module written in the 'old way'. It happens that you receive a comment asking ‘Hey, could you fix this TODO while you’re here?’ Should you touch it or not?
As usual, it depends. If it's a small and low-risk change, do it. Maybe not all of them if there are multiple, but just a few. Try to make the surroundings just a little bit nicer. If you don't do it, the code will slowly rot and turn into unmaintainable spaghetti over the years.
Sometimes the changes are too big to address them right away. What you could do is create a ticket or write down on your list 'refactors to be done while I'm bored'. And then when you have some slack time between two tasks just start grabbing things from that list.