Code Review Series: When to review code

How does your team handle code review? Do you assign each pull request (PR) to a specific person(s) for review? Or do you mark your PR as "ready to review" but not assign a specific reviewer? Do team members get any sort of notification when a PR becomes ready for review? Is that notification given via chat or email?

Do you like the way your team manages code reviews? What does your ideal code review process look like?

The Process On My Current Team

On my team, when a PR is ready for review, we assign reviewers to it via GitHub. Sometimes we assign one reviewer, sometimes we assign a few - depends on the situation. When we assign multiple reviewers, that means "any of these people can review this PR, whoever has time" - it doesn't mean "this PR needs to be reviewed by ALL of these people".

GitHub sends people automatic emails when someone assigns them to a PR as a reviewer. So my team's code review process depends on team members checking their email regularly. Which a lot of people don't, so I prefer to send people a message over chat when my PR is ready for review. (If we were in the office like back in normal pre-pandemic times, I'd tell them face-to-face, but these days everything is remote, so chat it is!)

Unless the PR is suuuuuuuuper non-urgent, then I just leave it to email and don't bother messaging over chat.

Often when someone on the team has a PR that anyone on the team could easily review (ie everyone on the team has enough context to understand the changes or the changes are extremely simple) they won't bother assigning a reviewer in GitHub and will just post a message in the chat.

My Ideal Process

I think in my ideal process:

1. I don't have to chase people down to get them to review my code. They do it in a timely manner of their own accord. I think maybe this has more to do with culture than with process. But if anyone has any process ideas for this, please let me know!

2. All developers on the team perform a roughly equal amount of code review. (By "amount", I mean time spent reviewing code, not number of PRs reviewed or amount of code reviewed or anything like that. But maybe this idea needs further refining.) No one feels like they are doing the bulk of the review work.

My Personal Policy

My personal policy for "when to review code" is:

1. If I finish working on a ticket, I do not pick up another ticket until I have given at least 1 round of review to all ready-to-review PRs within my team. Yes, you heard me - all ready-to-review PRs. Not just one. Finishing the work in progress takes precedent over starting new work. As my one friend recently told me, there is a saying that goes "work finished is more valuable than work started."

2. The first thing I do in the morning is to review any PRs that are ready for review. Before working on my current ticket or starting a new one. This is to ensure that code review happens. If I don't do it first thing in the morning, I may not do it at all that day because my current ticket might take up the whole day.

3. Maybe I should also consider doing code review when I come back from lunch.

What Not to Do

I see a lot of developers who think the goal is to crank out as many tickets as possible. And they seem to feel done with a ticket when they put it up for review. After that, they immediately move on to the next ticket. I urge you NOT to do this. You should not feel done with a ticket until it is merged. In other words, that ticket needs to be your #1 priority until it is merged (I think you should also feel ownership after that point - testing your changes in the next release candidate - and looking out for crash reports once the changes are shipped to production. But that's another conversation for another day.)

What to Do

When you put a ticket up for review, your next steps in order, are:

1. Review your own PR AGAIN. Even if you reviewed it before putting it up for review.

2. Make sure the team (or just the reviewer, depending on your team's process for code review) knows that your PR is ready for review.

3. Review any other PRs that are waiting for review. (You aren't done with this step until you have reviewed everything that is ready for review. In other words, this step is not done after you review just one PR. Unless there is only one PR that is ready for review.)

4. Catch up with email.

5. Do anything else you can possibly think of that is productive and contributes to your team's progress. Pair with someone. Prepare a presentation you've been meaning to give. Clean out your git stashes and old git branches.

6. Only then - when there is absolutely nothing else you can possibly do and you still haven't gotten a review on your PR - is it acceptable to start on another ticket. But remember - the first ticket takes priority. So as soon as you get a review on the first ticket, switch back to it. This new ticket is second priority. That is really, really important to understand.

Then, every day at standup*, you need to mention that your ticket is still waiting for review (potentially also mention how long it's been waiting for review). Ask in standup who will have time to review it today. Get someone to verbally commit.

*Standup is a morning "sync". It is also commonly called "scrum". It's intended to be quick, around 5 - 15 minutes. And it's intended to be done early on in the workday (pretty much first thing). At standup, everyone on the team takes turns giving a progress update (what did you do yesterday, what are you doing today, any blockers?) and that's it. Having a daily standup is a common practice in a lot of software development teams. It is part of the "scrum" methodology.