How long do you spend reviewing a PR?
I’ve noticed it can take me a while to review PRs. 2-3 PRs of about 10 files and maybe 500-1000 lines each can blow out 2-3 hours of my morning easily. I’m on a team of 5 engineers, myself included. I find I leave the most comments; more than anyone else on my team by a good margin. Mostly questions (40%-60%), suggestions (20-40%), and issues (0-5%).
Here’s my process:
- Read the ticket to understand the work that’s supposed to be done
- Read the PR line by line keeping an eye on certain things I care about
- - is it clean architecturally?
- - is the state being managed responsibly? Unreachable code? Missing null handling? Checking conditions that aren’t possible?
- - do the unit tests actually test all the cases it should or are we just going for green check marks?
- If my feedback isn’t going to change too much logic, I pull down the branch and test the code. Checking the AC and then playing around with edge cases to see if I can find something
3b. If my feedback is substantial enough that I feel it’s not ready yet for testing, I’ll wait for author and re-review when the time comes
The majority of my comments are questions that have spawned from me about to make a suggestion, but I don’t want to make an assumption so I always ask the author in case there’s something I’m missing
Instead of “this condition is unreachable, we can kill it” I ask “What scenarios do we expect the user to hit this condition?”
If I do make a suggestion or raise an issue, I won’t just say “change this”, I’ll jump into the code myself and pull references from our codebase to bring a solution to the table
“This can be improved if we follow a similar pattern in the code block below”
I pride myself in being very thorough on reviews; it’s one of my favorite parts of the job but I worry I spend too long on reviews; my coworkers are quicker to review, but leave less comments.
How do others perform their reviews? What can I practice to become better while maintaining quality?