Code review times two

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 is this 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 Some 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.