How to approach 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.

The full story is on my blog too.

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”.

Here are some tips I’m collecting about it.

Who?

CR is a team activity and, where possible, every member should join in it. 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.

As Author: preflight activities

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.

As Reviewer: mindset

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.

As Reviewer: what you should look for?

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.

Finally

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.

• Awesome Code Review: A curated list of tools, articles, books, and any other resource related to code review
• Code Review Best Practices by Palantir Engineering Team
• How to Give and Get Better Code Reviews by Tracy Lum
• Google’s Engineering Practices
• 10 Principles of a Good Code Review by Jason C. McDonald
• How to Make Good Code Reviews Better by Gergely Orosz
• Good Code Reviews, Better Code Reviews by by Gergely Orosz