Doesn't this depend heavily on the size of each PR? Sometimes it makes sense to have small changes, sometimes to have sweeping refactors. These would have vastly different times to complete a code review, for reasons unrelated to team productivity.
Doesn't this depend heavily on the size of each PR? Sometimes it makes sense to have small changes, sometimes to have sweeping refactors. These would have vastly different times to complete a code review, for reasons unrelated to team productivity.
Every time I've made a larger PR I've regretted it and subsequently split it up into smaller ones. I get higher quality feedback and faster feedback to boot that way.
Or you just get a "ship it" without more than a cursory glance.
The amount of active "eyeballs on code" time to complete a code review can of course vary tremendously. That said, when I see code reviews taking a week to complete, only very very very rarely is that a sign that developers are routinely spending a solid week of eyeballs-on-code time completing those code reviews. Most of the time it involves a relatively small amount of focused review time and a huge amount of not actually getting around to starting doing the code review time.
If everything in your shop is so well handled that you're not wasting any time waiting for code to get reviewed, in practice your shop is probably also already making good choices about how to structure code for efficient reviews as well (but that's more an anecdotal observation about correlations, not directly an observation about causality).
Someone once argued to me that each feature PR should have very few surprises if the feature has been planned and communicated well with affected parties before implementation even begins. The opposite is often true in some organizations.
I have also found that high review latency begets large code reviews. It's kind of a vicious feedback loop. If you always review PRs immediately, people are less afraid of splitting up their work, and PRs will become smaller. (It's hard to "stack" reviews, so people avoid it. That means that 3 PRs become 1 PR, so they can be mentally done with the task and start something brand new while waiting a week for your code review. The alternative is hoping that the bottom of your stack is approved without any requests to change anything, but what happens is that large changes are requested and the two floating PRs are now impossible to merge back in, or are irrelevant. If the bottom of the stack could be reviewed in an hour, then all that work wouldn't have been wasted, and it reinforces the good habit of doing 1 small thing at a time.)
I look at all of this as a systemic issue, and not a personal issue. The team needs to set an aggressive SLA, guided by what the engineer is expected to do while waiting for a review. Then engineers need to treat the objective seriously; code review is as important as any code you're writing. That's why you're part of a team, and aren't a 1 person army (which is a totally viable approach; I write a ton of code in my free time by myself, but I don't necessarily think that that's how I should behave at work.)
Personally, I have Github's Slack notifications on, and almost always start the review as soon as I get the notification. I know that the person waiting for the review has no work to do other than to build on top of this foundation; the less they do without feedback, the better. (Plus, they want to get their code Out There, that's why they wrote it.) I don't think your organization has to be that aggressive, but I'd aim closer to "hard interrupt" than "within 1-2 business days". (No, I don't review PRs that come in at 7PM on Friday night. Those wait until Monday morning.)
The other metrics listed a few comments above are really a function of organizational policies. Make your releases easy, so you do them often. Make sure that your PMs know what users want, so you don't implement code and throw it away. Make sure that CI is fast, so there isn't a delay in pushing to production. And make sure your process is realistic. My team used to spend a lot of time manually testing stuff in staging before releasing to production. It never found any issues that the test suite didn't find, and we prefer the automated tests anyway, so eventually we just made every commit to master a deployment to production. Click the "merge" button and it's out to the users. No unusual number of surprise production outages, just a vastly reduced time from idea to being in the hands of the customer. (Code reviews ended up being the limiting factor. People really don't like doing them!)