Healthy approach to code reviews

Code reviews are weird monsters. With no doubts they improve the overall quality of the code and help programmers build relationships and work together more effectively.

However asking for a code review puts many people on edge because it is just like laying your creative soul bare. By generally speaking most of the devs lives CRs as a “personal intrusion”.

Your team is only good as your weakest reviewer

Here are some tips I’m collecting about it in these years.

Who?

Code Review is a team activity. A good mixture of senior and junior members aim to create the right environment for learning: senior members can battle test their code related to APIs clarity, junior devs have a chance to learn new things.

Depending your team size and structure each review may require one or more engineering approval to be merged. From my experience if your team is pretty small (less than 5 people) you can stick with 2 reviewers to keep a good balance on sharing knowledge.

If you are growing over 8+ (without moving to cross-functional teams) you should change your approach: your CR should require at least one engineer and you should start moving the team to a more static organization where part of the program are assigned to an ownership which is also responsible for manteniance and evolution of that code (more specialized skills, less shared knowledge with a modular architecture which follow a shared vision – we’ll look at that in one of the upcoming posts).

Depending of the affected part you may decide to assign it to more people; I’ve seen it vary, for core layers you may considering sharing the knowledge across the entire team.

Help me, help you

We don’t have hard requirements for adding descriptions to pull requests but recently I found it very useful and instructive to use Templates. It help you to create a good habit both for the author and any reviewer.

First of all the author is allowed to take a deep breath and focus on what he did, describing the idea, the implementation and force himself to take an overview look to the PR.

That said, anyone who is reviewing code will need to figure out what is changing and why it’s changing.

In general, we think it’s good to consider what people may or may not know about your PR, and aim to give them context so that they can help you.

Small PRs is not a matter of LoC

I strive to build things in small units; when planning a new milestone my approach is the old divide et impera. I want to break our code down as much as possible and submit small, digestible PRs. By doing this, the team can review quicker and move faster.

However, when you think about a small PR you often refer in terms of lines of code (LoC). Instead you should think in terms of conceptual changes.

For example, a variable name refactor maybe a big review because it may impacts lots of files in your code base. However it still small because it’s just a single concept (the renaming).

In my engineering we values and encourages the combination of small PRs and quick reviews.

Author POV

As author of a merge request is your responsibility to avoid waste of time & energy of your collegues.

CR must be:

Highly scoped and limited: Changes should have a narrow, well-defined and self-contained scope. If it takes more than one hour think about splitting it.
Correct: It must handle the problem and should be performant enough for its use case; main edge cases must be handled correctly and tested.
Readable: Reduce the cognitive load necessary for an user to be productive with your code (it includes BL, naming & organization of the classes, functions and variables).
Elegant: Ask yourself “would you be proud and excited to work with this code?” As maintainer of the program you must leave the codebase better than what it was before.
Huge refactoring is a separate activity If you are planning to make a refactoring which touches many parts of the app consider it moving in another CR. Unintended behavior changes can leak into the code base without anyone noticing.

Reviewer POV

Your work as reviewer is only partially related to the tech side; you should put yourself into the correct mindset when approaching a new merge request.

Be nice: Always assume the best intentions: everyone wrote questionable code before, it happened, and it will happen again. Be kind and avoid downplaying words by using easily, simply or just… It just don’t serves no purpose.
Point out the good things: Don’t forget to praise concise/readable/efficient/elegant code. If you learned something new or interesting while reviewing let it the author know.
Take the right time: Truly understand the task and the code used to achieve it; once you are confident start writing your thoughts. Consider splitting long review sessions if it takes more than one hour (attention to details tend to drop easily).
Ask & Provide Context: If you see something you don’t know about you should ask for details; always provide alternatives, we could make it betterdoes not make any sense.

What you should check in a Code Review

When you are doing a CR you should focus to the following list of activities:

Purpose

The reason a code exists is to satisfy one or more business requirements.

Does the code you received achieve all the requirements?
Does it well support edge cases?

Implementation

Think carefully about how you would have solved the same problem.

If it’s different, why is that? Does your implementation handle more edge cases?
Is it shorter/easier/cleaner/faster/safer (yet functionally equivalent)?
Do you see potential for useful/not-over-enginnering abstractions

Clarity

When possible code must require a low cognitive load to be understood. Over-engineering is a particular type of complexity where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system.

As reviewer you should be especially vigilant about over-engineering to limiting future speculation.

Naming & Conventions

Naming and conventions are tools to reduce cognitive load. As team you should create, maintain and update your list of conventions; this set of rules does not strictly depend by technologies but also by the single personality of the team members.

By generally speaking you should keep in mind the following questions:

Are variables/classes/functions clear in their intentions?: You should be able to grasp the concepts and the flow in a reasonable amount of time.
Does the code follows your team code style?: Code should consistent with the rest of the project in terms of style, API conventions etc. Deviations could be accepted but must be discussed.
Are all comments into the code necessary?: Redundant and unnecessary comments clutter the code. Usually comments should explain why some code exists and rarely what is doing. If the code isn’t clear enough to explain itself, then there is room to make it simpler.
Handle TODOs: TODOs just pile up in code, and tends to become stale over time. You should be vigilant about their presence.

Tests & Docs

If your team maintain a test suits, as reviewer always ask for unit, integration, or end-to-end tests as appropriate for the change. Tests should be part of the code review unless author is handling an emergency.

Does the test cover interesting cases? Truly untestable features are rare, but untested implementations are too common.
Are tests readable? You should be able to understand what a test do.
Does this CR break backward compatibility? Think carefully existing production code and check if new code can break compatibility somewhere.
Was the external documentation updated? Outdated documentation is worse than none.

Key Takeaways

Keep in mind: code review is an asynchrnous activity don’t bother your reviewers with real-time communication unless you can’t come to an agreement or you think it’s necessary to the complexity of the changes.

Internet is full of articles about this topic; the links below are a short list of further readings where other details are discussed.