Ask HN: How does your code review process work?
We have about 15 developers working in permeable sub-teams, each merging one PR on average per day. When a PR is created, it needs to be CRed by at least one other developer, and developers should look at each others' work while they are waiting for their PR to be looked at.
2 comments
Morning routine: check email, check list of open PRs that I should review, respond to comments on my pending PRs, if any, and merge my own approved PRs.
Reviewing work by dev A: change looks good, but what is this condition for, given this other condition elsewhere? what does the alternative really mean? ask a couple of questions, request elaboration on a comment because we both know why it's there since we talked about it but the maintenance team won't have that context. Review responses to Qs in A's older PRs; looks great, approve.
PR from dev B: oh god, why write a two-line wrapper for a standard POSIX function and try to make a Linux-based app look like this is running on Windows? Check comments, oh god they all say "tbd". Request that new identifiers include some vowels, like regular English words; point out team's coding standard we all agreed to at the beginning of the project. Review responses to comments on B's older PRs; ah yes, he refuses to make any improvements and adds comments full of typos; hah, B also introduced the same typo in a previously correctly named variable; insist on improvements.
PR from dev C: that's weird, I don't get how that would work, let's compile it. Right, it doesn't even compile. Hmmm, two new classes, without comments, what are those for? right, they're imported but not actually used anywhere. What problem is this PR solving? no idea, everything is named aThing, ThingStorage, and FutureUse. Review responses on Cs older PRs; hmmm, no actual responses, just saying "will change in another PR". Wince, shrug, write a note to escalate to team lead at the end of the week, again.
Reviewing work by dev A: change looks good, but what is this condition for, given this other condition elsewhere? what does the alternative really mean? ask a couple of questions, request elaboration on a comment because we both know why it's there since we talked about it but the maintenance team won't have that context. Review responses to Qs in A's older PRs; looks great, approve.
PR from dev B: oh god, why write a two-line wrapper for a standard POSIX function and try to make a Linux-based app look like this is running on Windows? Check comments, oh god they all say "tbd". Request that new identifiers include some vowels, like regular English words; point out team's coding standard we all agreed to at the beginning of the project. Review responses to comments on B's older PRs; ah yes, he refuses to make any improvements and adds comments full of typos; hah, B also introduced the same typo in a previously correctly named variable; insist on improvements.
PR from dev C: that's weird, I don't get how that would work, let's compile it. Right, it doesn't even compile. Hmmm, two new classes, without comments, what are those for? right, they're imported but not actually used anywhere. What problem is this PR solving? no idea, everything is named aThing, ThingStorage, and FutureUse. Review responses on Cs older PRs; hmmm, no actual responses, just saying "will change in another PR". Wince, shrug, write a note to escalate to team lead at the end of the week, again.