6

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?

  • 2
    A little more detail would be helpful in answering your question. What development methodologies do you use? Do you work in sprints? How large is the team? Is everyone able to review anything, or do certain people take certain reviews? – Kyle McVay Apr 23 '19 at 16:44
  • 2
    Sharing your research helps everyone. Tell us what you've tried and why it didn't meet your needs. This demonstrates that you've taken the time to try to help yourself, it saves us from reiterating obvious answers, and most of all it helps you get a more specific and relevant answer. Also see How to Ask – gnat Apr 23 '19 at 16:45
  • Do developers follow the authored pull request until they are merged? – max630 Apr 24 '19 at 5:22
14

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.

  • 5
    To put it simply: if your people don't have time to review because all of their time is allocated to coding, you need to allocate less time to coding and more to review. It's really as simple as that. If you allocate 8 hours/day for coding, there will be no time for review. If you allocate 6 hours/day for coding, there will be 2 hours for review. – Jörg W Mittag Apr 23 '19 at 17:06
  • 3
    @JörgWMittag Yes, but I'd even go another step and say that if you allocate 8 hours to coding, you'll have -2.5 hours left in the day. No one's 100% productive or entirely focused on project work of any kind. – Thomas Owens Apr 23 '19 at 17:11
  • Thank you Thomas! I think that the problem lies in our definition of done and that it should be changed. – devoxy Apr 25 '19 at 9:42
  • One thing I would add is that it can be very effective to assign code reviews during sprint planning. Doesn't have to be set in stone, but everyone can have a rough idea of how much code/review work they have to do for the sprint. – Kyle McVay Apr 25 '19 at 15:59
  • @KyleMcVay That would require assigning work to people at the planning as well, which is antithetical to a pull system. – Thomas Owens Apr 25 '19 at 16:00
8

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.

  • Have an upvote for "Your development task is not complete until it's in production.". so many people think their job ends when the unit tests pass and it's just not true! – MarcE Apr 25 '19 at 13:19
7

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.

2

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.

0

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.