6

There is a school of thought that describes code reviews as the number one priority activity for all developers in a sprint since they are either:

a) A source of unrealised value

b) A source of bugs

While I'm not sure I subscribe to such as black and white view, I do agree with the sentiment.

But enough of the background.

We have a persistent problem where reviews sit on the queue for days at a time. Invariably the bottlenecks are caused by the lead developers as they're more in demand. Also, due to the important nature of their work, they tend to pair program so they don't require their own code reviews to be done as such. Hence there is no real incentive for them to do reviews as a quid pro quo.

The most often used complaint is that they will lose their train of thought while programming. There is some truth in this, but surely they could just review after lunch or first thing in the morning?

Are there any ways round this? On previous projects we have applied a SLA of one day which worked well for the most part but again, the rule tended to be broken by the pair programmers.

Developers take it in turns to do code reviews so the load is already being shared fairly.

Robbie Dee
  • 9,717
  • 2
  • 23
  • 53
  • 1
    I don't even understand what "a) A source of unrealised value" means. And I would hope that code reviews aren't a source of bugs. – Simon B Nov 20 '19 at 13:27
  • @SimonB Might just be a foible of my industry but we can't commit to the main branch until it has been reviewed, hence it cant be built, released, tested, supplied to the customer i.e. deliver *value*. – Robbie Dee Nov 20 '19 at 13:29
  • @SimonB A code review could reveal problems that require a certain amount of rework or even potential bugs. These will require development effort to remedy and so will have a negative effect on when the value can be realised. As the saying goes "fail early" – Robbie Dee Nov 20 '19 at 13:31
  • 2
    Possible duplicate of [How to get developers to do code reviews in a timely manner](https://softwareengineering.stackexchange.com/questions/158243/how-to-get-developers-to-do-code-reviews-in-a-timely-manner) – gnat Nov 20 '19 at 13:38
  • @RobbieDee There is NO good reason testing should be held up by the code not being in main. A feature branch can pull the current main at any time and should before the the merge anyway. What are you doing only testing it once it's main? – candied_orange Nov 20 '19 at 15:29
  • @candied_orange Talking about formal testing here i.e. handover to QA. Obviously the normal developer testing can (and does) happen... – Robbie Dee Nov 20 '19 at 15:42
  • *"they will lose their train of thought while programming ... surely they could just review after lunch or first thing in the morning?"* - it depends on the difficulty or subtlety of the work, and how long the train has to run for. Sometimes, the train has started before I even get to work, and continues into the evening, and nobody knows when it will reach its final destination, and the only person who can possibly determine when it's a convenient time for a break or switch of task is me. – Steve Nov 20 '19 at 15:45
  • @Steve Yep, good point... – Robbie Dee Nov 20 '19 at 15:47
  • More pairing. Then you can ditch the separate review, shorten the feedback cycle and integrate earlier. – jonrsharpe Nov 20 '19 at 17:14
  • @jonrsharpe That isn't possible or practical for a variety of reasons too long to go into here. – Robbie Dee Nov 20 '19 at 21:02

2 Answers2

9

The teams should be empowered to review their own code, after all there's no one better qualified.

GIT Pull Requests implement one of the easiest and most robust models for this:

  • Code cannot be merged to master until it's been reviewed by X (two is recommended) other people, usually there's a stipulation of one dev and one tester
  • Anyone can code review. A junior asking questions can have the same effect as a senior critiquing. It's about picking up complexity and edge cases.

There are a few things you can do to make it easier:

  • Pair programming counts as a code review (after all it was developed together)
  • Keep changesets small, there are many many many reasons why this is a good idea
  • Don't auto-assign to your most experienced, busiest people. Let other team members pick up code and let them take pride in the code base
  • Consider creating an extra state (or including it in the Definition of Done) for your stand up boards. Like all bottlenecks, make it visible so people can see work that needs to be done to unblock people
  • Consider using automated code review tools so people can focus on the important stuff, not the automatable rules
Robbie Dee
  • 9,717
  • 2
  • 23
  • 53
Liath
  • 3,406
  • 1
  • 21
  • 33
  • Some excellent points here - thank you. As a case in point, I'm waiting for a review of a CSV config file which has been live for weeks... – Robbie Dee Nov 20 '19 at 14:14
  • Especially the third bullet about auto-assigning is important. – Bart van Ingen Schenau Nov 20 '19 at 15:22
  • @BartvanIngenSchenau - agreed, I made that mistake when I became a tech lead. A pool is a far better approach than "send everything to me" – Liath Nov 20 '19 at 16:19
  • @BartvanIngenSchenau I think the fourth is as important in this case - as per the question, if reviewers don't see the need for reviews to take place, it'll get a lower position in their personal priority list. – Baldrickk Nov 20 '19 at 18:16
1

If you perform pair (or mob) programming, do you need peer review?

I've previously written an answer on the reasons why one would want to perform peer code reviews in the first place. However, all of these are also addressed by pair programming. In essence - pair programming is a real-time, highly interactive code review.

It may be easier to say that your code must either be pair programmed (and set up some team norms or guidelines around effective pairing techniques) or go through a code review, but both aren't required. Of course, you can always add a code review to get a third (or fourth) set of eyes on a change, but if you simply have a need to ensure that two people are watching and ensuring the quality of every change, pair programming gets you that.

Thomas Owens
  • 79,623
  • 18
  • 192
  • 283
  • Pair programming isn't a silver bullet (for us at least). We already have issues where one pair is waiting for another to do a review as one of the reviewers is a SME. – Robbie Dee Nov 21 '19 at 14:58
  • 1
    @RobbieDee Why does the work done by a pair need review? The pairing is the review. – Thomas Owens Nov 21 '19 at 16:51