Code Review

The problem: “looks good to me”

In our online shop, a developer added the “discount code” feature. They send a Pull Request with 900 changed lines.

Two reviewers look at it:

1. The first reviewer looks for five minutes and writes “looks good”. Approved.
2. The second reviewer writes twenty comments: an extra space, the order of the usings, the name of a variable.

A week later, a ten percent discount code is used a thousand times. The usage limit was never checked. Neither reviewer saw this.

Both reviewers spent time. But neither looked at the main question: does this code work correctly?

What is the goal of Code Review?

Code review has several goals. In order of importance:

1. Finding problems before users do. Logic bugs, security problems, cases that were forgotten.
2. Keeping the design healthy. Is the code in the right place? Can it still be changed a year from now?
3. Spreading knowledge in the team. Now at least two people know this part.
4. A shared style. The code of the whole team looks alike.

Leave code style to tools

Arguing about spaces and line order wastes people’s time and only puts everyone in a bad mood. A machine does this job better, and without taking sides:

1. The editorconfig file. It writes the code style rules in one place for the whole team.
2. The dotnet format command. It formats the code based on those same rules. In CI, you can just check if the code is formatted or not.
3. Code Analyzers. They show compiler warnings and quality rules at build time. You can turn important warnings into errors, so the build fails.
4. Tests in CI. If the tests have failed, a person should not spend their time yet.

<!-- Directory.Build.props: same rules for every project -->
<Project>
  <PropertyGroup>
    <Nullable>enable</Nullable>
    <AnalysisLevel>latest-recommended</AnalysisLevel>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  </PropertyGroup>
</Project>

Now the reviewer is free to think about the things a machine does not understand.

A real review

This is the main part of that same discount code Pull Request. Before you read on, look at it yourself. How many problems do you see?

public async Task<decimal> ApplyDiscount(Guid orderId, string code)
{
    var order = await _db.Orders.FindAsync(orderId);
    var discount = await _db.Discounts.FirstAsync(d => d.Code == code);

    order.Total = order.Total - order.Total * discount.Percent / 100;
    discount.UsedCount++;

    await _db.SaveChangesAsync();
    return order.Total;
}

A good reviewer starts from the base of the pyramid:

1. Correctness. The expiry date and the usage limit are not checked. This is the same thousand-uses bug.
2. Correctness. If the user clicks the button twice, the discount is taken off twice.
3. Correctness. If the discount code does not exist, the FirstAsync method throws an error and the user sees a 500 error, not an “invalid code” message.
4. Security. There is no check that this order belongs to this user. Anyone with an order ID can put a discount on other people’s orders.
5. Concurrency. If two customers use the last slot of the code at the same time, both succeed. Increasing UsedCount has no concurrency control.
6. Design. The discount rule is inside the service, and the order total is changed from outside. The rule must be inside the order itself.
7. Tests. There are no tests for expiry, the limit, and applying it twice.
8. Minor. The method name has no Async suffix and does not take a CancellationToken.

After the review, the code looks like this:

public async Task<IResult> ApplyDiscountAsync(
    Guid orderId, string code, string customerId, CancellationToken ct)
{
    var order = await db.Orders.SingleOrDefaultAsync(
        o => o.Id == orderId && o.CustomerId == customerId, ct);
    if (order is null)
        return Results.NotFound();

    var discount = await db.Discounts.SingleOrDefaultAsync(d => d.Code == code, ct);
    if (discount is null || !discount.CanBeUsed(time.GetUtcNow()))
        return Results.BadRequest("The discount code is not valid.");

    order.ApplyDiscount(discount);   // throws if the order already has a discount
    discount.MarkUsed();             // UsedCount is a concurrency token

    await db.SaveChangesAsync(ct);   // a second, parallel use fails here
    return Results.Ok(order.Total);
}

How to write a comment?

A good comment has three things: the problem, the reason, and how important it is. Talk about the code, not about the person.

A small label at the start of each comment makes the author’s job much easier. Some teams use a format for this called Conventional Comments:

1. Blocks merge. It must be fixed before the merge.
2. Suggestion. I think it is better, but the author decides.
3. Question. I did not understand. Maybe it is not a problem.
4. Minor (nit). Very small. If you do not want to, do not fix it.
5. Praise. Did you see good work? Say it. A review is not only about finding faults.

The author’s job: make the review easy

A good review starts with the author.

1. A small change. Each Pull Request should have one idea. Send renames separately.
2. A clear description. What changed? Why? How was it tested? Add a link to the related task.
3. Review it yourself first. Before sending, read the changes once like a reviewer. Many faults are found right here.
4. Point out the sensitive parts. “Please look carefully at the concurrency logic. I am not sure about it.”

The reviewer’s checklist

1. Behavior. Does the code do what the description says? What about edge cases? Empty input, duplicate input, very large input?
2. Errors. What happens if another service does not answer? Is an error silently swallowed?
3. Security. Is user input checked? Does a user only reach their own data? Is there a Secret in the code?
4. Data and concurrency. What do two requests at the same time do? Is the transaction correct? What effect does a Migration have on a large table?
5. Performance. Is there a query inside a loop? Is more data read than needed?
6. Design. Is the code in the right place? Was something built again that already existed?
7. Tests. Does the important behavior have tests? Does the test really fail if the code is wrong?
8. Readability. Will someone who reads this tomorrow understand it? Are the names in the language of the business?

Common mistakes