DEV Community

Discussion on: Code Review and your team

Collapse
 
isaacdlyman profile image
Isaac Lyman

"Days" is definitely too long, assuming there isn't a weekend or holiday in there. On my team I generally expect PRs to be reviewed within 1 working day.

A few thoughts:

  • Are you working to keep your PRs small and incremental? Giant PRs are daunting to review, even for extremely senior developers. In my experience the time required for a review increases exponentially for each additional line of code in the PR. If I'm going into a dev task I know will be big, I try to split it into two or more parts that I can PR separately.
  • How do you respond to feedback on your PRs? If you (or a previous team lead) have a history of being argumentative about suggestions and constructive criticism, that can make it draining for other developers to review your work. You may not even know you're coming off in a negative way; I've made that mistake before. It's a good idea to intentionally be extra positive and deferential with conversations like that.
  • Is your team proactive about documentation? On my team we'll commonly go through our own PRs (especially if we feel that they may contain confusing or unfamiliar design decisions) and comment each file for the reviewer's benefit. We also document our plans ahead of time, before writing any code, and discuss them with the team so we know what's coming.
  • Is your team under high pressure to perform and meet deadlines? If reviewing a PR for an hour puts them at risk of missing an objective and facing consequences at work, then leaving it for someone else to do is, understandably, their best move.
  • Is the rest of your team significantly more junior than you? They may be intimidated by your code, or think that they won't have anything valuable to say, or worry that they'll let an important bug through and be responsible for downtime in production.

I expect that if your team knows they should be reviewing PRs, but aren't, there's a psychological reason for it. And with some honest conversation and sustained change in team culture, it's solvable.

Collapse
 
wizofaus profile image
wizofaus

Generally I put up a PR that fills all the story requirements, if it's obviously too big, sure I'll break them up. As it is, I'm relied on to review almost everyone else's PRs regardless of size, which obviously takes up a sizeable chunk of my day, so I'm well aware of the danger of excessively big PRs.

As for commenting on my own PRs, yes I do that but I try to avoid it, if something needs explaining and it can't readily be done by writing clearer code, then the explanation is probably better off in the code comments. But if it's explaining, e.g, why I've deleted a bunch of code, then sure.

I wouldn't say our team is under unrealistic pressure, but the main reason we struggle to get things done quickly is often because PRs are waiting for review!

Understand about the "concerned-I'm-not-qualified" problem but I've repeatedly emphasised that it's primarily about having a second pair of eyes go over the code (plus an opportunity to familiarise yourself with the code-base etc.). None of our team are that wet behind the ears anyway, but obviously they have varying levels of experience in certain technologies.

Thread Thread
 
isaacdlyman profile image
Isaac Lyman

Sounds like your ducks are in a row then. Maybe it's just Bystander Effect? Maybe you can schedule a half hour with a specific dev and walk them through your PR, then ask for an approval right then and there.