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.

10
  • 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, 2019 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, 2019 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, 2019 at 13:31
  • 2
    Possible duplicate of How to get developers to do code reviews in a timely manner
    – gnat
    Nov 20, 2019 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? Nov 20, 2019 at 15:29

2 Answers 2

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
4
  • 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, 2019 at 14:14
  • Especially the third bullet about auto-assigning is important. Nov 20, 2019 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, 2019 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, 2019 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.

2
  • 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, 2019 at 14:58
  • 1
    @RobbieDee Why does the work done by a pair need review? The pairing is the review.
    – Thomas Owens
    Nov 21, 2019 at 16:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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