Assigning people to code review

Question

My team has a problem with making code review.

Everyone is constantly busy with their tasks and only few of developers do review on a daily basis. As every merge request needs 2 approvals, some of them stay in review for days if not for weeks (!), which slows down the project. Everyone understands the importance of it, but has their excuses.

How to assign people to code review or give them a boost to do it?

Answer 1

It seems like the problems begin with planning. When you determine the capacity of the team to take on a given body of work, you should consider the time that it will take to peer review the work done (in addition to everything else that is needed to complete the work). The idea that people are simply too busy to perform a required review prior to merging work is a symptom of problems upstream, in planning and estimation. I would reflect more on what you are doing in these areas and what can be done to improve their effectiveness.

The other thing to look at is why you require 2 approvals. I'm a firm believer in more eyes on the work, but what is this accomplishing? This also somewhat goes back to planning and estimation, but some bottlenecks may be relieved by understanding who is likely needed to review a piece of work and ensuring that the right people are reviewing it.

I don't think that you should be looking at your review and merge process for solutions, but rather the upstream activities and the overall discipline of the team to define the work, define what it means to be done, estimate the effort, plan for the team's capacity, and make commitments to each other to get it done.

Answer 2

Ah the fallacy of "I'm too busy developing to code review."

Well code reviews are development - just because it's inherently not your feature doesn't affect that.

Ultimately everyone suffers. It's not straightforward at all to get a culture of prioritising code reviews first but it is necessary.

Hence code reviews are priority 1 if assigned - only support issues can bump them. It's the reciprocation of reviews - being necessary for any task to be promoted through the release pipeline - is the motivation in and of itself. A good team should understand the importance of reviewing each other's work - not even having to wait for a formal code review to do it.

Your development task is not complete until it's in production.

There is never really "no time" to do something; just a misappropriation of what is available. As you are experiencing the overall slowing of the process is part of what causes "too busy." As such part of it causes its own problems.

Some of this may depend on the size of your team but if you're having daily standups no one really likes those. I'm a senior developer but I still stand up and go to talk to people working on stuff I'm going to review to see how it's going - especially if it's something I'm familiar with. Anyone you would consider senior should also be doing this - part of the role is to be interested in the overall process as well as developing more junior people. I also personally find it helpful to break from what I'm doing as slapping away at a keyboard isn't the most important part of development - it's thinking about what you're going to do. If that is the mode of operation your developers are in then they need to be rethinking that as well.

Answer 3

I would suggest you move to a KANBAN approach. eg:

   +----------------------------------+
   |todo    |dev     |coderev |done   |
   |        |max 3   |max 3   |       |
   +----------------------------------+
   |task 7  |task 4  |task 1  |       |
   |        |task 5  |task 2  |       |
   |        |task 6  |task 3  |       |
   +----------------------------------+

The key thing with KANBAN is that each column has a limit. If the column is full, you are not allowed to move the task into it. This is designed to highlight bottlenecks in production and show where more capacity is needed.

So in this case a developer has just completed task 4. The next thing they must do is to move the task into the code review stage. But they are not allowed!

They can't pickup task 7 and move it to dev as the dev column is also full. So they have no option.

In the next morning's stand up any blocked devs must move from developing to code reviewing.

Once the code reviews are done the column is freed up and the tasks can flow again.

Answer 4

Just make code review highest priority. If you see a request for a code review, you just walk to the nearest two people who are not reviewing anything, and ask them to do the review now.

A review delayed for a day is bad. Delay for weeks is seriously affecting the teams productivity.

Answer 5

This is where Pull Requests are your friend.

Work is not "Done" until it's merged into Master (actually it probably shouldn't be done until it's in Production but that's a whole other conversation). To get your work marked as "Done" so you can start the next piece it needs to be in Master. You need two CRs, it's the author's responsibility to get them.

You'll be surprised how much more willing people are to do CRs when they need their own done. Make it clear what you're looking for from CRs, make it easy to do them!

This is why I mentioned Pull Requests (a GIT thing) at the start. Out of the box you get:

• Quite a nice UI and easy way to do it
• Assurance that no one can commit into master without X approvers (you can even set areas and roles if you wish)

If your CRs are done after the work has been done then they'll always be seen as a bolt on. Equally if it's the Tech Lead's job then they become a bottleneck/enforcer. The CR and the Code aren't two separate jobs, they're both tasks of the same piece of work (just like testing and deployment).

In summary:

• You're not allowed to commit/merge without an approved CR
• Don't consider a CR a follow up, just like testing it is part of the required work to do the task