I like code review but complain about it all the time. Why? The other day I had an eureka moment: There are two quite different kinds, which I will give the misleading names agile review and waterfall review, and I'm annoyed when the reviewers perform one kind of review and I want the other.
Waterfall development is centered around planning,
development and deployment in stages: First plan well and get rid of
most bugs, then implement what's planned, then review and test to fix
the remaining bugs, then deploy. In this model, code review focuses on
catching minor bugs, ensuring code style harmony, proper use of
internal/thirdparty APIs, etc. Big questions such as
feature a good idea at all? are out of scope, because that's been
settled in the planning phase. (I'm not saying waterfall works in
practice, only describing the theory.)
Agile development is different. Instead of planning well, agile development is based around an iterative process: Implement something that seems to be a good idea, consider it, learn something from it, finish it and then deploy, then pick something else to do, something one now knows more about. Code review can be the first part of the consideration and learning process, so it's a good time to discover that although a feature seemed like a good idea, it's going to be a morass and should be dropped at once.
An example may show this better. Consider a server that serves requests from clients and uses a sharded database backend, and a customer who complains that although performance is fine in general, a few requests are much too slow. The reason for this hypothetical problem is that combining results from different shards is too slow.
In an agile process, the team might open an issue saying
requests are too slow, see support ticket 123456 and bring 95th
percentile response time closer to 50th. The developer
investigates, write some code, see that it works on the happy path,
and ask for review. In this case, the first thing to discuss during
code review is what problem is solved by the change and whether it is
the ultimate problem or more of a bandaid. Next, perhaps, how to test
this approach. Only after that does it make sense to write the tests,
implement the sad paths, review the code style, level of testing and
other implementation details.
In a waterfall process, one person does the investigation and opens
an issue saying something more like
access database shards in
parallel so that performance is better when many shards are needed for
one request, and the questions for code review is whether the
implementation conforms to the style guide, whether the tests cover
the problem and will avoid false positives/negatives, etc.
These two kinds of review discuss different questions, and they even work best on different code. An agile review should start much earlier than a waterfall review: If you're going to discuss whether you've understood the bug at all, then it makes sense to discuss that before you bother with 100% unit test coverage. If the code's going to be dropped there's no point in getting 100% test coverage and fixing every corner case.
The waterfall review is based on a much more firmly worded
issue. There's little scope for learning about the problem in
access database shards in parallel…, so no point in
doing the review until the code is complete and all the aspects of the
review can be done at the same time.
I wrote above that the names are misleading. What I call waterfall review can be necessary in an agile process. A very agile team where I once worked had the freedom to modify or drop almost every feature as we learned about the problem space. But only almost every feature. There was a contract that demanded that we interoperate with a named third-party service using a specific protocol. Using that protocol was a poor choice, that protocol should be buried, not used, but it wasn't our choice to make.
Most code related to that third-party service had to be reviewed in the waterfall manner, not our usual agile style. I remember a terribly frustrating review where a reviewer wanted to do his usual excellent agile thing and it just wasn't helpful. The contract was signed, the details were fixed and the discussion could not affect anything.
The opposite mistake is more common: Asking waterfall questions even though the team is supposed to work in an agile manner. I think the main reason for that is that the reviewers want to write code, and a waterfall review doesn't require mental immersion in the way an agile review does. Just scan for style aberrations or other surface issues, ask for another code revision and get back to work.