Historically, the first code review process that was studied and described in detail was called “Inspection” by its inventor, Michael Fagan. This Fagan inspection is a formal process which involves a careful and detailed execution with multiple participants and multiple phases. Formal code reviews are the traditional method of review, in which software developers attend a series of meetings and review code line by line, usually using printed copies of the material. Formal inspections are extremely thorough and have been proven effective at finding defects in the code under review.
Anyone who has done code reviews in the past can see how easily it is to convert them into hard discussions. Pull request is a type of code review in contexts with lack of trust, so they affect team morale. A lot of times they are a tool to preserve “Status Quo” not a tool to improve quality in our product. Bureaucracy and trust are things related, as less trust you have, more bureaucracy you introduce in the system. Bureaucracy means long times to solve problems but doesn’t mean to do a better job, just to use more time to evaluate things. But bureaucracy also means more waste, introducing more waiting times in the system, so money burnt. The question then is how can we build more trust and what type of code reviews help us to do it?
Pull Requests (PRs)
PRs are a feature introduced by GitHub in their product from the very begining (2008). GitHub allows developers all around the world to collaborate in a distributed way in products. GitHub and Pull Requests are key to understand open source projects nowadays.
Pull Requests are pre-merge async code reviews. So the code review is done before the code is merged into the target branch. It is done through the differences in code between the source branch which is modifying the code and the target branch, where the code will be merged. Apart from the fact that they are using branches, they are also async by nature. Reviewer and reviewed are working in isolation and they communicate through the PR, they are not together.
The conversation is going to happen eventually. Reviewer will read the code and comment those parts unclear for him/her. Reviewed will be informed that reviewer has some comments. Then reviewed will try to solve them to present a new version solving these issues.
Basically, Pull Requests are async pre-merge code reviews. They are not thought to optimize lead time (latency between devs starts developing until that code is in prod), Pull Requests introduce big delays to have code deployed to production. Remember Pull Requests != Code Reviews. In an open source scenario as described above. If lead time is crucial for a feature, the core team will do the review.
I usually say that they are a great way of doing code reviews on environments where you cannot trust in the people who write the code. Why, because of all the properties described above, they are gatekeepers, they try to avoid introducing bad changes in the project.
They are great for having a core team in charge of the project whose mission is to protect the product to create.
Let’s imagine an open source project done by hundred of volunteer devs all around the world, the core team has to be sure no one of those devs have introduced malware in their codebase.
Pull request is a type of code review in contexts with lack of trust. So they are great for big open source projects where trust is impossible.
Development teams
Development teams are the usual way to change code in companies, they have been created with the idea that if people collaborate together they will achieve better and faster results.
The main focus of a development team is collaboration, we should optimize our ways of working having this in mind.
The idea of teams comes from collaborative sports as football or basketball, and to collaborate, trust is a must.
You need to trust in your teammates, you have to accept that you cannot compete alone against the other team, so you cannot run always to get the ball, you just need to trust that your teammates will help you on that.
If trust is important I will argue that a development team needs to see each other with a high frequency, can we build good dev teams in isolation, I don’t think so. Can we create development teams in remote?, I think so, we have tools to be in touch all the day.
I think that the baseline to have a team is having all of them in a similar timezone, being able to talk the same language, and collaborate together as much time as we can.
Apart from trust we create development teams to reduce the lead time, we want to be as fast as we can to provide our new features to our customers.
So why is it going to be a good idea to use in development teams a process created for untrusted environments? Why is a good idea to use a process where lead time is not a priority?
Lead time and Little’s law
In mathematical queueing theory, Little’s law (also result, theorem, lemma, or formula) is a theorem by John Little which states that the long-term average number L of customers in a stationary system is equal to the long-term average effective arrival rate λ multiplied by the average time W that a customer spends in the system.
Where WIP = number of tasks in progress Throughput = average departure rate, number of stories per week for example LeadTime = average time a task spends in the system
Lead time doesn’t refer to the time we spend on a feature. But rather the time it remains in the system. In our case, the system is our route to production.
Path to production = The steps to put the feature in front of our customers + all the waiting time.
Post Merge Code Reviews
Post merge code happens after the code is in main. We can do post merge codere reviews using a specific a step in our path to production. The code review is just a new state before considering the story finished. Summarizing a lot, the code review needs to happen to consider something as done.
Using the “commit message” will help us to identify commits for a specific feature.
Post merge code reviews are a signal of trust. I mean, you are allowing your teammates to merge their code in main because you trust them. Youo trust on the ways they work, their intentions, their knowledge, etc.
It’s important to be sure that the developers care about what they build. So they are the main responsible for their changes. This is another characteristic of trust, when you trust someone you also make him/her responsible for the things done. Automatizing simple feedback is usually faster than waiting for people. Then let’s agree on tools to format our code, let’s create fitness functions to protect some architectural characteristics, etc.
Post merge code reviews can be sync or async. If we want to reduce our Work In Progress, it is much better use sync reviews. This means to have reviewer and reviewed in the same computer talking about the changes. In async mode, reviewer checks the code and writes to the reviewed about changes. If we agreed on some changes it is better to trust they will be done, not doing another review once they have been implemented.
The idea here is to do the code review with the reviewer and reviewed together. So they can talk about the changes at the time the dev has finished them.
Pull Requests are an option. But we cam also do pre-merge code revies without branches at all, reviewing the code in the computer of the developer before merging to main.
This technique remove some of the delays introduced by the async nature of pull requests, but it requires doing code reviews frequently in the team, to avoid pilling up code reviews. Every day for example you can have a time for code review scheduled in the calendar where everyone is checking the code done by others.
Collaborative code reviews.
In fact, they are the natural thing that happens when you reduce the batch size to commits. Creating multiple PRs per hour will make reviewer and reviewed to work in the same computer, just because it’s cheaper. Pairing is in fact a method to do continuous code review. It increases trust because working together will make people to find agreements at the right moment. Remember Pull Requests != Code Reviews.
Mob programming follow the same principle at higher scale. Reducing the WIP to one and making all the team to collaborate.
You can think that pairing or mobbing are going to slow down your team, but it is just the opposite. They are ways of working, based on collaboration, that reduce waste time to the minimum. Coming back again with the Little’s Law and your team:
LeadTime = WIP/Throughput
You can argue that is much more effective to paralellize work, but this is just true when that work doesn’t need to integrate. We tend to minimize the integration cost, it’s not like playing lego but to transplat organs.
“The devil is in the details” so too much work to integrate when devs work in isolation will be a difficult task.
Let’s don’t delay integrations. We can integrate small parts frequently even if those parts are incomplete. We should not underestimate the cost of integrate different parts done by different people during days in isolation. Integrate small things frequently, will have a high impact in the quality of the product. Waste time is against lead time, a lot of wast time means unfrequent integrations. This is why pairing or mobbing will help to create a better product sooner.
Code reviews try to improve the quality of the product created. But quality is a property of the system. It’s better to add quality at earlier stages not at the end. Pull Requests != Code Reviews.
Cease dependence on inspection to achieve quality. Eliminate the need for inspection on a mass basis by building quality into the product in the first place.
