How to do code reviews correctly

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.

AI-generated image by Bing mimics a saying that's believed to be Plato's words:

"Let none but geometers enter here" which depicts how important it was for philosophers to know mathematics.

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

1. Read the requirements well, so you understand the problem the PR is attempting to solve.
2. 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.
3. 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.
4. 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.

When doing composition, forget about what you inherited!

      I'm revisiting the first chapter from Head First Design Patterns, which is my favorite one so far.

It promotes the composition against inheritance through the strategy pattern, and to put it together, here are the main points of the two approaches as I got from the chapter.

Choosing full inheritance (no polymorphism) may be good if you are looking for 

- Code reuse: Code can be written once and shared among subclasses.

but you should think twice before using inheritance for only that purpose because, on the other hand, this can be at a cost, because:

- Every subclass will share the "default" behavior, with no possible customization, classes lose the singularity advantage.

The first solution that comes to mind is employing polymorphism through using interfaces or abstract classes, that fixes the full inheritance issue so:

Behavior can be customized (any subclass can have its own version of the base class/ interface).

But here are the issue with polymorphism:

- Change is costly, where you have to go through all implementations to change the behavior.

- There may be duplications.

So the strategy pattern comes into play if you want to avoid the red points.

The problem is that you don't know if your implementation needs full inheritance or be overridable. because when inheriting implementation from the base class you're stating strongly, that the subclass will never change in a different direction, and when using polymorphism you're stating that the subclass will always be different than the base class, the problem is that; you're always referencing the base class in a way or another: changes are done with respect to the base class.

In the strategy pattern behaviors of subclasses have their own personality that shouldn't be tied to the base class⁽¹⁾, and you compose them into the subclasses and you can change this behavior in runtime.

Let us have the example used in the HFDP book:

you want to implement different types of ducks in the app, each with fly and quack behaviors,

so here's is the IFlyBehavior

public interface IFlyBehavior
{
    void Fly();
}

IQuackBehavior:

public interface IQuackBehavior
{
    void MakeQuack();
}

The Duck abstract class:

    public abstract class Duck
    {
        public void Display()
        {
            Console.WriteLine($"{Name} Duck");
        }

        public IFlyBehavior flyBehavior;
        public IQuackBehavior quackBehavior;
        public abstract string Name { get; }

        public void Fly()
        {
            flyBehavior.Fly();
        }

        public void Quack()
        {
            quackBehavior.MakeQuack();
        }
    }

For every type of duck, there may be different fly/quack behavior, with the strategy pattern, we create the needed behavior class, implementing the IFlyBehavior or IQuackBehavior and attach it to the concrete duck class, for example, the DecoyDuck implementation looks like this:

public class DecoyDuck : Duck
{
    public DecoyDuck()
    {
        flyBehavior = new FlyNoWay(Name);
        quackBehavior = new MuteQuack(Name);
    }

    public override string Name => "Decoy";
}

this fixes the inheritance issues, except for one issue I found later,  which I named the title of this blog post after, do you see it?...

It's the Name property being translated to the behavior class from the base class:

        flyBehavior = new FlyNoWay(Name);
        quackBehavior = new MuteQuack(Name);

with this translation, all the benefits of the composition are put in danger because now the supposedly inheritance-free components are no longer free, they are tied to the base class with this little weight. 

I found this issue when I wanted to change how I attach behaviors, so instead of initializing behaviors in the subclass constructor, I wanted to add them from outside through two public methods: SetFlyBehavior and SetQuackBehavior to change behaviors in runtime. and this means I will need to send the duck name, for example:

var duck = new DecoyDuck();
duck.SetFlyBehavior(new FlyNoWay("Decoy"))

I'm stating Decoy twice as the type and as the string name, I didn't feel OK with this, the name of the duck is loosely coupled with its type, and they need to be maintained together.

The fix is simple; I removed the name from the IFlyBehavior and made it a separate component without being connected to the duck properties. with this change, I can compose any behavior without worrying about the Duck class.

This is all about it, a good lesson on how strategy pattern with composition can be employed.