Code Review != Pull Requests
There are a lot of people whose first and unique relation with code reviews is through pull requests. So they think this is the only way…
There are a lot of people whose first and unique relation with code reviews is through pull requests. So they think this is the only way to do it, no, it is not.
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.
https://en.wikipedia.org/wiki/Code_review
The Fagan Inspection was first published in IBM in 1976, as you can see code reviews are an old thing in software.
Pull Requests (PRs)
They are a feature introduced by GitHub in their product, they are there since GitHub started (2008). GitHub allows developers all around the world to collaborate in a distributed way in products. I would say that 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 thought 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 based on branches, they are also async by nature, the code review is not done with the reviewer and reviewed talking about it at the same time.
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 and 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 since it is in prod), Pull Requests introduce big delays to have our code merged.
In an open source scenario as described above, if lead time is crucial for a feature, it is usually done by the core team.
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 goalkeepers, they try to avoid introducing bad changes in the project.
They are great for having a core team in charge of the project who watch for some properties of the code.
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.
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 we will achieve better and faster things.
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 at that level 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, where the trust is impossible?.
Why is a good idea to use a process where lead time is not a priority?.
Post Merge Code Reviews
A post merge code review is done after the code is in main. I did this for a while, and you can easily control that every story has a code review through your path to prod. The code review is another step that needs to be done before considering any story finished. Summarizing a lot, things are not finished because they are in prod.
Adding in the commit message a label to understand that that commit is because of that story is enough to do the code review through commits.
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 on the ways they work, their intentions, their knowledge, etc.
There are some requirements to work in this way, the main one is to be sure that the developer needs to be sure enough their changes are not introducing bugs or problems in the system.
As there is no one else that is going to tell them this is right, this is wrong, the only way to have that feedback is through automatic tools.
Automatizing simple feedback is usually faster than waiting for people.
Post merge code reviews can be sync or async, if we want to reduce our Work In Progress, it is much better to do them synchronously.
This means to have reviewer and reviewed in the same computer talking about the changes, the why's and eventually what needs to be changed.
In async mode, reviewer check the code and write to the reviewed about the changes to be done. In my experience, it is much better to not review again once the changes are done, it is better to trust, if not we are again with the back and forth.
More about post merge code reviews, here.
Pre-merge sync code reviews
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 can be used here, but also they can be done without branches at all, reviewing the code in the computer of the developer previous to merge it 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 PRs to a level that makes reviewer and reviewed to realize that it is much more productive to be together writing the code at the same time as reviewing it.
Pairing is in fact a method to do continuous code review, it increases trust because it is basically to collaborate on doing things together and reduce lead time to its minimum expression.
Mob programming follow the same principle at higher scale, reducing the WIP to one, making all the team to collaborate in all aspects of work to put the features in front of our customers.
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 a minimum.
To understand this, Why Don’t Ants Get Stuck in Traffic?
Spoiler: because they are always talking and collaborating to achieve the goal, they are not alone.