Clean Code

The problem: code that only its writer understands

In our online shop, one method calculates the final price of the shopping cart. This method works, and the tests are green too:

public decimal Calc(Order o, Customer c, string? code)
{
    decimal t = 0;
    if (o != null)
    {
        if (o.Lines.Count > 0)
        {
            foreach (var l in o.Lines)
                t += l.P * l.Q;

            if (c.Type == 2)
                t = t * 0.9m;

            if (code == "NOWRUZ")
                t = t - 50_000;
        }
        else
        {
            return 0;
        }
    }
    else
    {
        throw new Exception("error");
    }
    return t;
}

Now the sales team says: “From this month, the VIP customer discount is 15 percent.” A new developer must change this code. There are a few problems:

1. They must guess. Does the number 2 mean a VIP customer? Is the number 0.9 the discount?
2. They are afraid. They do not know what else breaks if they change one line.
3. They do not see a hidden bug. If the cart is very cheap, the coupon code makes the price negative. In this mess, nobody notices.

Code is read much more than it is written. Every time someone reads it and gets confused, the team loses time and money. Clean code means code that is cheap to read and change.

A good name: half the work

A good name answers the reader’s question before they ask it.

A few simple rules for names:

• A name must say the purpose. Instead of t, write subtotal. Instead of d, write daysSinceLastOrder.
• Use business words. If the sales team says “VIP customer”, write IsVip in the code too, not Type equal to 2. This is the shared language from the DDD lesson.
• A class is a noun, a method is a verb. The PriceCalculator class, the CalculateTotal method.
• Write a boolean value like a question. Like IsVip, HasStock, CanCancel.
• Do not use abbreviations. Writing a long name takes time once. Reading an unclear abbreviation takes time every time.
• No meaningless names. Words like Manager, Helper, Data and Info say nothing. Ask “what exactly does this class do?” and use that as the name.

Small methods, with one job

A good method does one job. If you use the word “and” to explain what a method does, it probably does two jobs.

Why is a small method better?

1. The method name explains the code. When you put a piece of code in a method named ApplyCoupon, nobody needs to read it line by line any more.
2. Each method talks at one level. The main method talks about “sum, discount, coupon code”. The details of the loop and the multiplication are in the lower methods.
3. Testing gets simpler. You test each small rule on its own.
4. A change has a clear place. The VIP customer discount changed? Only the ApplyVipDiscount method changes.

Two points about parameters too:

• Fewer parameters are better. If a method has five or six parameters, usually some of them together form one idea. Put them together in a record.
• Remove boolean parameters. A call to Place with the value true is hard to read. What does this true mean? It is better to make two methods with clear names, like PlaceAsDraft and PlaceAndPay.

Exit early (Guard Clause)

Nested conditions push the code to the right, like an arrow. The reader must keep all the conditions in mind to reach the main work.

The solution is simple: send the special cases out first. Is the input empty? Throw an error right at the start. Is the cart empty? Return zero right at the start. After these few lines, the main work is written with no indent at all.

Magic numbers and duplication

A magic number is a number in the middle of the code that has no name. Like the number 2 or 0.9 in the code above. Give it a name:

private const decimal VipDiscountRate = 0.10m;

public enum CustomerType { Regular, Vip }

public sealed class Customer
{
    public CustomerType Type { get; init; }
    public bool IsVip => Type == CustomerType.Vip;
}

Now if the discount rate changes, you change only one place. If the same number was repeated in three files, one place might be missed.

Duplication has the same danger. If a business rule is copied in several places, on the day the rule changes, one of the copies is forgotten. But be careful:

No hidden side effects

A method name promises what job is done. A method must not do more than its name says.

public Cart GetCart(Guid customerId)
{
    var cart = _db.Carts.Find(customerId);
    if (cart is null)
    {
        cart = new Cart(customerId);
        _db.Carts.Add(cart);
        _db.SaveChanges();
    }
    return cart;
}

public Cart GetOrCreateCart(Guid customerId)
{
    // same body as before
}

There is an even simpler rule: a method that returns something should not change anything. A method that changes something should return only the result of the work. At a larger scale, this same idea is called CQRS.

Comments: “why”, not “what”

Good code itself says what it does. A comment is for what the code cannot say: why.

// Bad: repeats the code.
// Subtract the coupon amount from the total.
total -= coupon.Amount;

// Good: explains a decision the code cannot show.
// Finance asked that a coupon never makes the price negative.
total = Math.Max(0, total - coupon.Amount);

• A comment does not replace a bad name. If you want to put a comment next to a variable, first make its name better.
• An old comment is worse than nothing. The code changes, but nobody changes the comment. After a while, the comment lies.
• Delete dead code. Do not keep commented-out code. Its history is in Git.

The full code after cleaning

The same method from the start of the lesson, after all these rules:

public sealed class PriceCalculator
{
    private const decimal VipDiscountRate = 0.10m;

    public decimal CalculateTotal(Order order, Customer customer, Coupon? coupon)
    {
        ArgumentNullException.ThrowIfNull(order);
        if (order.Lines.Count == 0)
            return 0;

        var subtotal = SumLines(order);
        var afterVipDiscount = ApplyVipDiscount(subtotal, customer);
        return ApplyCoupon(afterVipDiscount, coupon);
    }

    private static decimal SumLines(Order order) =>
        order.Lines.Sum(line => line.UnitPrice * line.Quantity);

    private static decimal ApplyVipDiscount(decimal amount, Customer customer) =>
        customer.IsVip ? amount * (1 - VipDiscountRate) : amount;

    // Finance asked that a coupon never makes the price negative.
    private static decimal ApplyCoupon(decimal amount, Coupon? coupon) =>
        coupon is null ? amount : Math.Max(0, amount - coupon.Amount);
}

Notice a few things:

1. Now the sales team’s change is one line. Only the value of VipDiscountRate changes.
2. The hidden bug was found. When ApplyCoupon was split out, the question “what if the price goes negative?” came to mind by itself.
3. The coupon code is no longer a fixed string in the code. Now it is a Coupon object that comes from the database.
4. The unclear error was removed. Instead of an Exception with the message “error”, we have a precise, standard error.

Team habits

Clean code is not the job of one person. A few habits help the whole team write clean code:

• The Boy Scout Rule. Every time you touch a file, leave it a little cleaner than before. A better name, a smaller method. You do not need to fix everything in one day.
• One format, automatic. With an editorconfig file and the dotnet format command, a tool checks the code format. Arguing about spaces in code review is a waste of time.
• Keep it simple (KISS). The simplest way that works correctly is usually the best.
• Do not build what is not needed (YAGNI). Interfaces and settings for “maybe one day we need it” make the code heavy.

Common mistakes

How far should clean code go?