Am I the only one that thinks this is acceptable? Sometimes you have a large change, and you can’t reasonably split that up into something smaller. We have tests to give us confidence that whatever the change did, it didn’t irrevocably damage the system.
To be fair, after things get into the master branch there’s still about 2 months of manual QA review for all releases, but if this weren’t the case we’d simply have to increase test coverage.
No, you’re not, and yes, sometimes you have and you can’t. (Assuming you meant reviewing 500 lines at once, not summarily passing a huge change without really reviewing it at all.)
If someone is implementing something that is inherently complicated, I much prefer to see the complete picture and do one thorough review than be drip-fed little pieces for a series of smaller reviews. That way, I can take the time to understand what is really happening, and look for potential dangers like:
• misunderstandings about requirements
• missing cases
• inconsistencies between related cases
• deviation from expected protocols or data formats
• deviation from established patterns or architecture
• unnecessary duplication
• useful abstractions that have been overlooked
• sequences or combinations that can lead to an invalid state…
Code reviews of tiny partial changes tend to offer little benefit in my experience. Sure, maybe I’d spot a typo or something, but you don’t need me for that, it’s what your linter is for. The real value of a skilled human reviewer is recognising more fundamental dangers like the ones above, and it’s really hard to spot those when you can’t see the forest for the trees.
It doesn’t help that many diff/review tools emphasize code that has changed so much that almost everything else is often hidden by default. It’s not unusual for the majority of the feedback I give in a code review to depend on related code that wasn’t changed, because reading that is often when you realise something is inconsistent or has been overlooked.
This is also why I don’t mind reviewing 50 or even 500 lines of code, as long as those lines make sense together. It’s not as if I’d ever only look at 5 lines for most 5 line changes anyway. Yes, it can take time, but the whole point of code review is to be another person who has taken enough time to think about the code and maybe come to some different conclusions, which might lead to useful discussions and maybe making some changes.
But isn't this what design review is for? So you don't end up in a position where you suddenly have a 500 line change set based on misunderstood requirements, and you have to review the code to find out?
Many of the things you mention should be caught in design review, and then even more of them would be caught in an early draft review (say 50–150 lines of code) with most of the actual implementation stubbed out.
That way, when you finally get to the big 500 line full implementation, almost all of the high-level concerns have been worked out at an earlier (cheaper!) stage so only minor comments remain, and those it is effective to be drip-fed.
Even with useful reviews at the design stage, I would still always look out for those kinds of issues as part of a code review, though. The approach might have evolved during implementation, or that implementation might have unintentionally deviated from the intended design. Also, sometimes the code is doing something inherently complicated and there just isn’t much to see until a whole algorithm is substantially implemented.
I personally save many-hundred-line reviews for 100% automated refactorings or string find/replace or similar, though I acknowledge I'm often doing extra work just to make the PR process smoother (even if it's a throwaway intermediate step).
That's if your engineering leadership cares about testing. In my experience that is a big "if".