Code review: how experienced developers do it?

High-quality code review is one of the secrets of the most effective teams. Over the years I noticed a few interesting ways to approach it. Now I share them with you in this article.

Recipe 1: Suggesting refactoring in the right moment

Seemingly nothing spectacular, though I do not mean excessive code polishing just to make it look as the reviewer likes it. It is rather about noticing that code change was hard to introduce and incurred extra technical debt. A professional sees that if the developer first refactored the code, then actual change would be much simpler. Plus, no technical debt run-up. Woohoo!

Example 1: A developer writes a very similar code to one that already exists (or copy-pastes because why not #YOLO). A reviewer suggests deduplicating it by moving it elsewhere so it can be used in both places.

Example 2: A developer needs to handle another case, so they change a function to accept 3 arguments instead of 2. The last is a boolean flag which cuts the function in half with an if-statement. A reviewer notices that if the developer would create a new class for new business logic, the function would remain simple and the burden of decision-making could be moved to the new class.

Recipe 2: Suggesting more efficient ways of solving a problem

This can manifest itself twofold - either by proposing a more concise solution (fewer lines of code, thanks to e.g. more idiomatic solution) or one that will work faster. First, let's say BIG NO to optimization maniacs that would rewrite the project in assembly if they were let to.

Example 1: A developer implemented a solution using a for-loop with nested if-statement. A reviewer suggests rewriting code using list-comprehension with filter (if).

# First approach
available_employees = []

for employee in employees:
    if employee.is_working():
        available_employees.append(employee)

return available_employees

# Second approach, after code review
return [employee for employee in employees if employee.is_working()]

Example 2: A developer uses a constant list of values to check if a variable is in it. Reviewer #1 suggests using tuple to make search more optimized. However, guessing is not an engineering practice and should not be combined with performance. Reviewer #2 benchmarks list, tuple and set to prove that the latter is the right choice:

# original code
ALLOWED_STATUSES = [
    'NEW',
    'WAITING_FOR_RESPONSE',
    'OVERDUE',
    'REOPENED',
    'ESCALATED',
]

def can_be_opened(ticket):
    return ticket.status in ALLOWED_STATUSES

# benchmark using timeit module
## list
python3 -m timeit -s 'ALLOWED_STATUSES = ["NEW", "WAITING_FOR_RESPONSE", "OVERDUE", "REOPENED", "ESCALATED"]' '"NON_EXISTING" in ALLOWED_STATUSES'
> 5000000 loops, best of 5: 61.5 nsec per loop

## tuple
python3 -m timeit -s 'ALLOWED_STATUSES = ("NEW", "WAITING_FOR_RESPONSE", "OVERDUE", "REOPENED", "ESCALATED")' '"NON_EXISTING" in ALLOWED_STATUSES'
> 5000000 loops, best of 5: 56.1 nsec per loop # Negligible improvement

## set
python3 -m timeit -s 'ALLOWED_STATUSES = {"NEW", "WAITING_FOR_RESPONSE", "OVERDUE", "REOPENED", "ESCALATED"}' '"NON_EXISTING" in ALLOWED_STATUSES'
> 20000000 loops, best of 5: 15.6 nsec per loop  # 4 times faster!

In the end, the developer may not even use set because, for God's sake, that's searching a 5 items-long list in a non-critical place!

Recipe 3: Cloning code locally to review it

A little thing, but enables to see code changes in a broader context. While Github/Bitbucket/Gitlab etc. provides a convenient way to review code with highlighting changed lines, such way of browsing may also prove itself useful in certain situations.

Recipe 4: Paying attention to acceptance criteria

No, I don't mean glancing over code to see if it does what the ticket says in the title (too obvious!). I mean literally reading the ticket first, then taking one acceptance criterion at a time to see if it is fulfilled.

My mind was blown when I saw it the first time. Thanks to my ex-colleague, Domi! :D

Example: A developer writes code for calculating shipping price. A reviewer reads the ticket and at acceptance criterion #4 sees there is a rule for free shipping if the total is more than $200. Then, they request changes to be made :).

Seriously, this should be a starting point for many if not all code reviews. Too often I saw developers focusing only on technical aspects without paying attention to the reason why the code was written in the first place.

Recipe 5: Reading tests first

Ahh, test-first approach at its best - also for code reading, not only writing!

This is another take on recipe 4 - paying attention to acceptance criteria. Not only a reviewer sees if tests are present (their absence can be a first reason to block PR...) but also if they describe all acceptance criteria specified in the ticket.

Recipe 6: Paying attention to names

Naming things is hard and often subjective. However, senior developers should simply have more experience with it (or they managed to read entire Wikipedia to know a lot of words with meanings). As a result, they can often name things more concisely and precisely. Experienced people (AKA those who have been working with the project for a while) will also guard naming - prevent synonyms from happening to make sure it is easy to understand code.

Example: A developer uses its own discovered synonym for the name of the variable. The synonym is long, has French roots and is sooo sophistiqué. A reviewer knows that in the project there is already a convention of naming that thing and politely asks to use a name common in the project.

Also, check out Ubiquitous Language.

Bonus! A dark side of code reviews:

anti-recipe 1: Not being able to review logic if they don't like your indentation or formatting

Yes, it is important to maintain a uniform way of code formatting. But making comments only about the way how a developer placed braces is unproductive. What I also observed is that it is unlikely that someone will focus as much on logic if they see flaws in indentation (seriously...).

To avoid a lot of such discussions, quickly implement a code formatter (e.g. black for Python) + linters.

anti-recipe 2: Using code review to impose own style of coding and problem-solving

For the last, disheartening issue with code reviews, namely using it to make sure everyone writes code in the way a reviewer likes. Usually, it would be the most senior developer on the team. Such behaviour is toxic and often roots in overgrown ego. To make matter worse, such people often become bottlenecks since everything has to be blessed by them. Truth to be told, if the people have some fundamental knowledge and work on the project for some time, code they deliver will be good enough for most of the time. There is no need to make them rearrange every line. :)

The real role of the code review

You may think that the goal of the code review is to teach others. Well, not precisely. That's merely a side effect, though much appreciated by everyone!

Code reviews are to prevent bugs, by catching them as early as possible. It is way cheaper (and faster!) then waiting for a QA engineer to return it to you.

What are your secrets for doing effective code review? :) Share in comments!