A very common thing I see on development teams is reliance on code reviews for every check-in (in this age of Git-Everything, often referred to as “Pull Requests”). This can create bottlenecks in the delivery process, as our peers are pulled away from their own work and we have to wait for their feedback. And, often, the end result is that these reviews are superficial at best, missing a tonne of problems while still holding up delivery.
But why do we do these reviews in the first place?
I think of it in programming terms. Imagine a web service. It has a number of external clients that send it requests via the Web.
These client apps were not written by us. We have no control over their code, and therefore can’t guarantee that the requests they’re sending will be valid. There’s a need, therefore, to validate these requests before acting on them.
This is what we call defensive programming, and in these situations where we cannot trust the client to call us right, it’s advisable.
Inside our firewall, our web service calls a microservice. Both services are controlled by us – that is, we’re writing both client and server code in this interaction.
Does the microservice need to validate those requests? Not if we can be trusted to write client code that obeys the contract.
In that case, a more appropriate style of programming might be Design By Contract. Clients are trusted to satisfy the preconditions of service calls before they call them: in short, if it ain’t valid, they don’t call, and the supplier doesn’t need to waste time – and code – checking the requests. That’s the client’s job.
Now let’s project these ideas on to code reviews. If a precondition of merging to the main branch is that your code satisfies certain code quality preconditions – test coverage, naming, simplicity etc – then we have two distinct potential situations:
- The developer checking in can be trusted not to break those preconditions (e.g., they never check in code that doesn’t have passing tests)
- The developer can’t be trusted not to break them
In an open source code base, we have a situation where potentially anyone can attempt to contribute. The originators of that code base – e.g., Linus – have no control over who tries to push changes to the main branch. So he defends the code base – somewhat over-enthusiastically, perhaps – from bad inputs like our web service defends our system from bad requests.
In a closed-source situation, where the contributors have been chosen and we can exercise some control over who can attempt to check in changes, a different situation may arise. Theoretically, we hired these developers because we believe we can trust them.
I personally do not check in code that doesn’t have good, fast-running, passing automated tests. I personally do not check in spaghetti code (unless it’s for a workshop on refactoring spaghetti code). If we agree what the standards are for our code, I will endeavour not to break them. I may also use tools to help me keep my code clean pre-check-in. I’m the web service talking to the microservice in that diagram. I’m a trusted client.
But not all developers can be trusted not to break the team’s coding standards. And that’s the problem we need to be addressing here. Ask not so much “How do we do Pull Requests?”, but rather “Why do we need to do Pull Requests?” There are some underlying issues about skills and about trust.
Pull Requests are a symptom, not a solution.