alt Hacker News> But also because humans aren't all that great at code review when it comes to correctness and security. Part of the problem is that you don't have access to the author's mental model for non-trivial changes and have to reconstruct it, otherwise your understanding of the program logic will be inadequate.
Code review is not really about correctness and security. We have testing and other verification process for these. Code review is about idea discussion mostly and correcting a few stuff that may have slipped through the author's initial review.
Your PR wants to change the software in some way. My review is to comment on how you've implemented that change, like did you miss some areas where you needed to change, does your description is actually what is happening (either a cursory reading or through testing), or are you doing some unnecessary change.
IMO, the initial reading shouldn't take more than 10 minutes. More than that (so a big diff) it should have been discussed upfront, and the design agreed. If it's an important area, a more careful reading should be done about the tests and relevant stuff.
But generally, if a review takes more than 10 minutes, there's some failure of communication going on (reviewer not apt to understand the change, big diffs that should have been discussed outside of the PR, obtuse code,...)
Replies
> IMO, the initial reading shouldn't take more than 10 minutes. More than that (so a big diff) it should have been discussed upfront, and the design agreed
Part of the code review process is to verify that the writer actually did build the agreed design too though.
All good points. Maybe having worked a lot with concurrent and distributed system makes me distrustful of relying on unit/integration tests for correctness; I've seen too many concurrency issues pass CI validation. But unit tests also sometimes miss edge cases in more straightforward code, so I'm still a bit more cautious.
I also agree that merge requests should be short and understandable in 10 minutes, but IME this happy state often does not obtain in practice. It shouldn't, but it often does, whether it's mission creep, whether it's turning out that a change is more complex than originally imagined, or whether it is unavoidably complex and cannot be broken down further.