Introduction
Code review is a special event in the life cycle of software development, it's where ownership is transferred from the developer of the pull request to the reviewer.
This ownership implies fulfilling special conditions for the successful delivery of the piece of requirement - the ultimate goal of our craft.
For me, and it's a personal preference, the most I like about this phase (from a developer and reviewer perspective) is the transparency of the mind work, seeing the manifestation of ideas, and exchanging them, a practice that rarely takes place in a such a fast-paced world, which I enjoy, and I believe once you find the joy in it, you'll see code reviews differently.
Roles in code review
To understand the importance of the code review process, we should define and understand the roles involved in it.
Machine-side
Machines are at the heart of our industry, whether you view them as just tools or basic players, they are a fundamental part of the picture, and we can't ignore them even in a topic about a humane practice like code review.
Machines simplify this process by mandating specific requirements that are not subject to change between code reviews, these requirements such as running unit tests and ensuring they pass and ensuring coding standards are applied, etc. This can lead to a faster and more effective code review process, focusing on our part, not the machine part.
Human side
Two human roles (no surprise) coexist in the code review process: the developer and the reviewer, each dictates within his ownership boundaries. These boundaries combined complement the collective ownership of the workpiece. In a perfect world these combined ownership boundaries are not owned by anyone, it's rather what defines an organisation.
There is no advantage here, being the developer doesn't put you in a lower rank than a reviewer, and being the reviewer doesn't put you in a higher rank also. Speaking of the ownership language, there is no higher or lower ownership value, a team member transfers the ownership to the next member. How they value this ownership and adhere to it, is the important factor here.
Developer
Ownership for developers implies that they take full responsibility for the code, from meeting the specifications to adhering to the organization’s coding guidelines and best practices.
Since I'm only interested here in the code review process and the interaction between the developer and reviewer, I won't expand on this phase, as it's a different topic. The interaction starts right before you set your working space free from local commits and push them to the remote server for the code review. It starts from this point because a special first code review takes place, where you, the developer, check out your changes locally in the diff viewer, to look at your changes from another angle raising the bar a little bit, doing some criticism to your work as if another eye looking and reviewing your changes. Once you are comfortable with your changes, the second code review takes place.
Reviewer
This is a special role in the code review process. The reviewer is who makes code reviews an effective process or is just a guard to the gate to the target branch. An effective code reviewer can massively influence the team members, establishing a culture of communication, quality and ownership, as all these values are exercised during the code review. It's even more important in async-style communication environments.
What's expected from you as a code reviewer
- Read the requirements well, so you understand the problem the PR is attempting to solve.
- For non-trivial PRs, use your preferred IDE to go through the code. Using features IDEs offer can help you understand/ navigate the code better.
- Be collaborative, and take ownership of the code as it is yours. When suggesting improvements it might be also good to provide the code with your suggested improvements, this will cut short the guesswork and put you both at the heart of the problem.
- Test the code as you can, you may be more experienced in the domain than the developer, and not all test cases are covered by unit tests. Your colleague will thank you for catching potential bugs before they slip through production.
Here are some points to keep when you code review:
Your tone should be persistent across PRs: develop your neutral language, and use it with all developers with any experience level. This implies you are motivated by objective reasons, not subjective ones.
Be ready for questions: typically, developers receive questions about decisions they have made, but when you are more experienced as a reviewer, your comments can raise questions from developers, be ready for them, and don't just rephrase your comments but rather answer the question in a more detailed manner.
Prefer questions over assumptions: rather than commenting on a decision the developer had taken by making assumptions, ask them and clarify your point in the question. Questions create conversation, assumptions create justification.
When the reviewer is less experienced than the developer
That is an important result when it's adopted that code reviews are an ownership transitioning process, not a seniority-based process. Anyone in the team can do code reviews, including developers at any experience level as long as they take ownership seriously, which is, again, not related to their experience level.
However, to be an effective process for both sides, it's important (as a less experienced developer in the reviewer role) to pick PRs that you can take ownership of, and not be very hard to follow.
Don't be afraid to ask questions, and don't be overwhelmed by how many things you aren't aware of. For new joiners, reviewing PRs is a great way to learn about the domain, the stack and all the new things you find yourself in.
Experienced developers in the leading roles, should make new joiners feel comfortable reviewing PRs, welcoming their suggestions and extending discussions whenever there is an opportunity to do so in the code review. This can help establish an ownership spirit for new joiners.
The interference of roles
There are no strict boundaries between developer and reviewer, actually, in the code review process, and for effective communication, the roles are just rotated, the developer needs to view his changes from a reviewer's point of view, and the reviewer needs to view the changes from a developer point of view as if he would have done it.
Here are some points to consider (as developer and reviewer) in code review:
Don't leave questions open: Trivial as it may seem, you may forget to answer a question or respond to a comment for several reasons, for example, if a private conversation had been carried out between both of you- developer and reviewer, you may think since the point has been cleared in the conversation there is no need to answer the question in the PR comments. But this is not true, it's been cleared for both of you but for a future reader, it's not.
Stay in context: Code review is more than a private dialogue between two developers, it's a contribution to the knowledge base that belongs to the collective intelligence of the organization along with the formal documentation. Moreover, code review is a critical point in the life of the feature, so it makes sense that our conversation helps in this mission by staying within its boundaries.
Keep your conversation in the context of the PR, express your ideas clearly, reference relevant work items as needed, and don't assume others understand what you do.
Be kind and friendly: it's a conversation between two persons, and we want it to flow and to be effective.
Prefer communication over PR, over private conversation: splitting conversation between the PR and external private chat session harms the context of the PR badly, and makes it hard to rely on the PR as a standalone source of information for any future need, but when a single discussion thread grows unexpectedly, consider moving to a private chat session, then add the conclusions you have both agreed on to the PR discussion.
Use text formatting correctly: know the text formatting tools the platform you are using offers to enhance your writing. Most platforms offer a help link beside the text editor for the text formatting shortcuts, for example, code format, links, quotes, etc.
Formatting text makes writing clear, easy to read and follow which in turn results in smoother communication.
For example, one of the important formatting tricks is the combination of link and code format. For example, if I want to refer to the AddAuthentication method API (provided by Microsoft) in a comment, I'd use this format:
[`API link text`](URL to the API), i.e.:
[`AddAuthentication(IServiceCollection)`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.authenticationservicecollectionextensions.addauthentication?view=aspnetcore-8.0#microsoft-extensions-dependencyinjection-authenticationservicecollectionextensions-addauthentication(microsoft-extensions-dependencyinjection-iservicecollection)).
This is the resulting link that will appear in the discussion:
Here, instead of showing a very long and distracting URL (that is itself irrelevant to the context of the discussion), I'm only showing the piece of information that matters, with a hyperlink that optionally offers more information, and is also enclosed in a code format (using the backtick symbol `) to denote that it's an API not just a hyperlink. It's one of the standard formats in almost every software-related platform (the above image was taken from Stack Overflow).
In some other contexts, though, it might be useful not to hide the URL by formatting, when the referenced URL holds important value, or the URL itself is short and descriptive. For example, see both usages of referencing a GitHub issue and PR in a comment:“There is an open issue regarding a Postgres integration bug in the provider: https://github.com/org/repo/issues/135”
And
“If the issue with Postgres integration was resolved, I think we are safe to work on this issue, can you please pick it up @developer?”
Can you tell the difference between the two occurrences of the GitHub link (issue and PR- both are fake links)?
In short, sense the significant value- the attention direction.
In the first comment, we want to draw attention to an unexpected event, that is, the bug in the 3rd party library, which is causing issues in our service. The link is short enough and tells there is an issue in the library by just looking at the link in the comment, no need to open the link to understand what the commenter means, and it's the ending of the comment, so no distraction.
In the second comment, we were already aware of the issue, the commenter confirmed it was fixed (you can follow the hyperlink if you want to learn more) but the significant matter here is that we need to work on our issue, since nothing is stopping us now.
You may think these are all minor details, but these aesthetic details form and enrich what I defined previously: the context.
Conclusions
We defined the roles and understood the dynamics of code reviews. Aside from our obligation to do code reviews as a non-negotiated step in the software delivery process, code reviews are a great place to build an accumulative experience while focusing on your work. It's nearly impossible to keep track of all the ongoing changes to the system. It's an effective way to catch up with all the updates you are not directly involved in. Spending little time doing code reviews (compared to the time spent in developing the PRs) would give you a comprehensive experience in a fun and interactive way.
Credit to Paul Trees
I'm grateful to be working alongside a magnificent tech lead from whom I learn too much, like how to do code reviews correctly.
Lots of lessons I discussed here are my observations of Paul's reviewing process. I remember when I joined the team I was so amazed by his reviews that I asked him gently to do a live code review for one of my PRs to see him in action, I believed he was hiding some unseen magic!
In this article, I wrote about his magic in code reviews.
