How To Foster An Effective Code Review Process

What is a code review?

A code review is the process to review the code of your co-worker. It is usually done on a platform (like Github or Gitlab), for visualizing what changes in the codebase.

Why do a code review? 📝

There are many benefits to do code reviews. Some of the benefits are:

• Better quality: catch bugs or edge cases before going to production
• Higher velocity: everyone will be more comfortable with the codebase seeing more of the changes
• Learning opportunities for team members: both for reviewers (learn pattern, functions, etc) and reviewees (coaching)
• Foster a positive engineering culture: aim at quality work and reward it is a healthy goal, by improving readability, modularity, and more.

‍

‍

Excuses to not do code reviews

I think most of the people writing code do know about code reviews. But not all of them are doing code reviews or have their code reviewed. What are some excuses for not doing it?

‍

Time

We don’t have time

If you work 7 hours in a day that may be rephrased by:

We chose to not dedicate time to it

Let’s estimate the time it takes. If the scope of the change is reasonable, a thorough code review may take up to 20 minutes. But you don’t need to review every other pull request. If all the engineers are involved in the process (as it should be) you may do one or two pull requests per day. That is 10% of your time.

You may argue that there is also the reviewee time modifying his code. But this time is actually a gain. Time spent on improving the code base, that’s the time not lost by a co-worker doing it later without the context.

Considering the benefits it brings, that is not much.

We don’t have time because there is a rush

I know, it happens. It depends on what we call “a rush”. To me a rush is exceptional. It can’t be the last two days of every sprint.

Example of a rush: it is linked to a financial outcome like the ability to close a very important client or getting a round of funding for a startup.

Example of a "fake" rush: a manager expects the thing to be done because a non-technical person commits to an unrealistic deadline.

Anyway, not doing code review during a rush is technical debt. And technical debt is a great tool to use. It just has to be clear that, like financial debt, it has to be repaid (with interests, the more you wait the higher they are).

‍

Seniority

I am too junior to make code reviews

or

I am not confident enough to approve code reviews (because I’m too junior)

The good news is that you don’t need to be a senior engineer to start doing code reviews. And the more you do it, the better engineer you will be. You should start as soon as possible.

There may be a misunderstanding of what is expecting from a reviewer. A code review is not only approving the change from an architecture point of view. For example, readability is as important as modularity. Something very easy is reviewing variable or function names. Do they make sense? Do you understand what the code is doing by reading them? Are they specific enough?

This is something engineers of any level can do. And it is so important, for future evolution or for debugging. Questioning variable names can also make you think of potential improvements. What if you can’t have a specific name for a function? Or you come up with a name like getRowsAndSetColumns. It is now obvious that you probably need two functions instead of one.

Your co-workers know the different engineering levels in the team. Approval from a very new person on a critical change is maybe not the same as from the person that created the app. People know that. And it’s more the responsibility of the reviewee to ask code reviews from more senior people if necessary.

If a team member can’t review a code change because he or she is too junior, that’s a problem. It means not everyone in the team is in the same boat and there will be bottlenecks and heterogeneous quality. Having the code approved by the most senior person in the team is nice. But having it by the most junior is a good sign too.

‍

‍

How can I start? 🕵️‍♀️

Start small, do you understand variable names? Ask real questions, to understand, try to learn from it (thinking “I would not do that, why did he or she do that?” Maybe you can learn a pattern or a convention). Is there an edge case (from real life) that can break the code. Is there automated tests. Is there hard coded variables. Is there something you find particularly well done, you can say it too.

And if you feel confident to modify this code in the near future, approve.

If you are new to the team you will probably have many comments on your code reviews. So you will easily get inspired and you can start from here when you review.

‍

‍

What can you say, what should you not say?

I highly suggest reading some advice, on how to make comments. Many of them are also applicable to professional communication in general but if you are not sure, it can give you some confidence. For example simple things:

• Avoid linking code to a person: “your code does that and it’s a problem”, “you shouldn’t do that”. Remember, you are not your code. The code someone wrote can be good or bad but it doesn’t mean the person is. You can say “this function does that and it’s a problem” instead. Or “I think we shouldn’t do that”, including you in the project, “we” instead of “me” vs “you”. That’s general communication advice in the workplace, similar to when you discuss ideas with people. Focus on the idea and not the person behind it.
• Prefer asking naive questions rather than prescript. I prefer realizing there is a better option myself than just being told what to do. This is more sustainable too. The reviewee has more context, he or she may even come up with a better solution than you (typically a variable name).

‍

‍

A few tips that work

You will find exhaustive checklists of things to do in a code review. A few things I recommend, or that I discovered lately and find interesting.

‍

Either “approve” ✅ or “request for changes” ❌

But don’t be in the middle. On Github, there is an option to just “comment”. I encourage people to not use it.

Usually, it’s when people don’t dare to “request for changes”. It is important to have a culture where it is normal to not approve pull requests on the first shot. No matter who the author is: a lead, a senior engineer, or the CTO.

You should be surprised that your pull request has not been “requested for changes” and not the other way around.

Avoiding the “comment” option leads to real discussion and back and forth between the reviewer and the reviewee. Because now you need the approval of this particular person.

Go fast ⏳

It is important to review pull requests on a reasonable timeline. When you submit a pull request you are still fresh about it and changes can happen quickly. You want to move on as fast as possible and not have to update the code that you wrote last week.

That will also limit the number of current pull requests at the same time and thus help to avoid merge conflicts.

It does send the message to your co-worker that you care about their work.

How fast? A day seems a reasonable timeline, if a pull request appears in the morning, for example, try to have a look before the next morning. And once you asked for changes, if the reviewee does the changes, it is nice to go even faster to approve. Don’t interrupt your work but as soon as you get a slot to check Slack you may check it.

This means, for most of the code reviews, they should be merged within 2 days maximum.

‍

Scope the changes and stick to it

I know it may be tempting sometimes to do more in one pull request like update a package version (“it is easy and it will not break”) or fix this small annoying bug. But try not to do it. I have seen this advice that I liked:

Refactoring changes should not alter behavior; conversely, behavior-changing changes should avoid refactoring and code formatting changes

Either a change in the behavior or a refactoring. A few advantages:

• It’s easier for the reviewer to understand the meaning of one change only
• It’s easier to identify bugs and revert changes
• It doesn’t block one feature because there is a debate on the fix that wasn’t in the initial scope

‍

Respond to every comment ✍️

Your co-worker took the time to review your code. You have time to respond to his comments.

Usually, not responding can happen when you agree and you do the change. But it is nice and easier to follow up to have a “done” as an answer. It means all the other comments are not done and need either a discussion or an explanation.

It also personally helps me to track what I have done from the rest, as I write “done” when I do it.

‍

On comments and TODO ⚙️

The general advice is to avoid comments in the code unless they are absolutely necessary, which is rarely the case.

Sometimes, a working feature is put on hold: “can we hide this feature, we will add it again when…”. Features on hold rarely come back as is, and even if they do, that’s why we use versioning like Git. So don’t comment this code, remove it.

Sometimes, comments transform themselves in “TODO”. A different kind of comments you can track in your IDE. A good tip I have seen is to attach a ticket to the TODO you are creating and link them. I guess it forces you to ask yourself if this is really necessary. If it is, you will not forget it because it is now in the backlog.

‍

On documentation 📚

I mention it because that is so easy to forget to update the documentation doing a change. And once one thing is not up to date, why would someone update it.

It is similar to automated tests, it won’t be easier “later”.

‍

‍

Start now

There is a lot you can learn out there to improve your code reviews, but the sooner you will start and practice with your team the better you will become at it. I recommend to re-read advice after a while doing code reviews, as you will see all the things you do already, some things you can still add to your process and also things you don’t agree too much with.

Let me know what your code review process looks like and what you recommend to increase code quality and foster an inspiring engineering culture.