4 ms·
In my experience, code reviews are more about knowledge sharing than correctness checking (unit tests and CI are better direct correctness checks). You prioriti
by beliu 8y ago
In my experience, code reviews are more about knowledge sharing than correctness checking (unit tests and CI are better direct correctness checks). You prioritize understanding the code that has changed and hope you get correctness as a side-effect of that understanding. If you try to directly construct an exhaustive correctness checklist, you'll end up producing a CR process that's overly burdensome and that no one actually follows. So create a checklist for understanding the code, not checking correctness.
Here's the rough, hierarchical checklist I wrote up for myself at some point: https://gist.github.com/beyang/b3b494f028996b32ab3dae237bc1c2eb https://gist.github.com/beyang/b3b494f028996b32ab3dae237bc1c...
I don't go through this entire checklist for every code review obviously, but whenever there's a diff that's large/complex enough for me to say "hoo-boy", I revisit it to remind myself of what I actually need to check in order to obtain an adequate understanding of the change.
The other thing that matters is tooling. One of the annoying things about most CR interfaces is they lack the capabilities of an IDE that actually let you jump around and make sense of the code in a tractable manner. Shameless plug: I helped build a browser extension that adds these capabilities back into most of the mainstream CR systems: https://docs.sourcegraph.com/integration/browser_extension https://docs.sourcegraph.com/integration/browser_extension. People swear by it and I'd highly recommend trying it out.
To get back to your original scenario, it seems like you asked the right initial questions, but stopped short of building a satisfactory understanding of what was going on. You took the author's word for it, when you could've asked a few more probing questions to check your (and their) understanding of the change. At the end of the day, the threshold for "LGTM" is a function of both how well you understand the code and how much you trust your teammates. Better process and better tooling can help out a lot with the former, but the latter is a human element that can't be ignored either.