Clean code rules that actually survive code review, shown on one real pull request

Every team says it wants clean code. Fewer agree on what that means when a specific pull request is open and someone has to approve it. After many years of reviewing backend code, I have a short list of rules that rarely start arguments and reliably make the next change easier. This post shows them on one real pull request from our billing service, lightly anonymised.

The pull request

The task: when a subscription renews, apply any unused account credit to the renewal invoice before charging the card. Here is the function as first submitted:

def process(s, d=None):
    # process renewal
    if s.status == "active":
        if s.account.credit > 0:
            amt = s.plan.price
            c = s.account.credit
            if c >= amt:
                s.account.credit = c - amt
                amt = 0
            else:
                amt = amt - c
                s.account.credit = 0
            inv = Invoice(s.account, amt, d or date.today())
            inv.save()
            if amt > 0:
                try:
                    charge(s.account.card, amt)
                except Exception:
                    pass
            s.account.save()
            return inv
        else:
            inv = Invoice(s.account, s.plan.price, d or date.today())
            inv.save()
            try:
                charge(s.account.card, s.plan.price)
            except Exception:
                pass
            return inv
    return None

It worked in the happy path and the tests passed. It still received eleven review comments. Below are the rules behind them, then the version that was merged.

Rule 1: names say what, at the level of the caller

process, s, d, amt, c. The reader has to hold a mental lookup table. Good names cost nothing at runtime and save every future reader a few seconds, many times over.

What we ask for in review:

• Functions are verbs that describe the outcome, not the mechanism: renew_subscription, not process or handle.
• Variables carry units or meaning when it is not obvious: amount_due_cents, credit_applied.
• Short names only in short scopes. i in a three-line loop is fine. c across thirty lines is not.
• No type in the name (invoice_obj, user_dict). The type checker already knows.

This rule survives review because it is easy to check and easy to fix. Nobody argues for keeping c.

Rule 2: a function does one level of work

The original mixes three levels: business policy (renewals apply credit first), arithmetic (how much credit to use), and infrastructure (saving rows, calling the payment provider). Each change to one level forces you to read the others.

The test we use: can you describe the function in one sentence without the word "and"? If the sentence is "renews the subscription and computes the credit split and saves and charges the card", it wants to be split.

We do not use a line limit. A 40-line function that does one clear thing is fine. A 12-line function that mixes policy and I/O is not.

Rule 3: error paths are explicit and loud

The worst lines in the original are these:

try:
    charge(s.account.card, amt)
except Exception:
    pass

If the charge fails, the invoice is saved as if paid, credit is consumed, and nobody is told. This is the comment that blocked the merge. Our rule:

• Never catch a broad exception and continue unless you log it with context and the caller can tell something went wrong.
• Catch the specific error you can handle, and say what handling means: retry, mark invoice unpaid, notify.
• Let everything else propagate. A crash with a stack trace is better than silent data corruption.

Rule 4: comments explain why, not what

# process renewal repeats the function name. It adds nothing and will be wrong the day the function changes. Comments that survive review explain decisions the code cannot express:

# Credit is applied before tax because our tax provider treats account credit
# as a discount, not a payment. See the 2025 VAT review in the billing docs.

That comment tells the next person why the order matters and where the decision came from. If they want to change it, they know what to check first.

Rule 5: flat control flow, early returns

The original nests four levels deep and duplicates invoice creation and charging in both branches. Early returns for the edge cases, then one straight path, removes both the nesting and the duplication.

The merged version

def renew_subscription(subscription: Subscription, on: date | None = None) -> Invoice | None:
    """Create the renewal invoice, apply account credit, then charge any remainder."""
    if subscription.status != SubscriptionStatus.ACTIVE:
        return None

    account = subscription.account
    price = subscription.plan.price_cents
    credit_used = min(account.credit_cents, price)
    amount_due = price - credit_used

    # Credit is applied before tax because our tax provider treats account credit
    # as a discount, not a payment. See the 2025 VAT review in the billing docs.
    invoice = Invoice.create(account, amount_cents=amount_due,
                             credit_applied_cents=credit_used,
                             issued_on=on or date.today())
    account.consume_credit(credit_used)

    if amount_due == 0:
        invoice.mark_paid(method=PaymentMethod.CREDIT)
        return invoice

    try:
        charge = payments.charge(account.default_card, amount_due, idempotency_key=invoice.key)
    except payments.CardDeclined as exc:
        invoice.mark_unpaid(reason=exc.code)
        dunning.start(invoice)
        return invoice

    invoice.mark_paid(method=PaymentMethod.CARD, charge_id=charge.id)
    return invoice

What changed, mapped to the rules:

Two improvements came for free. Money is now in integer cents instead of floats, which a reviewer spotted once the names made units visible. And the charge now uses an idempotency key, which was hard to see a place for in the nested version.

What did not survive review

Some "clean code" advice we tried and dropped because it caused more argument than value:

• Hard function length limits. They produce artificial splits like _renew_part_2. We review for one level of work instead.
• "No comments, the code should explain itself". Code explains what. Only comments and commit messages can explain why.
• Extracting every expression into a named helper. credit_used = min(credit, price) is clearer inline than behind compute_credit_to_use().
• Banning early returns. Single-exit style made our functions deeper and harder to read.

Making it stick

Rules that only live in reviewers' heads get applied unevenly. What helped us:

• Linters for the mechanical parts. Ruff catches broad except with pass (rule S110 from the bandit set, and BLE001 for blind excepts), unused variables and overly complex functions, so humans do not have to.
• A short review checklist in the pull request template with these five rules, one line each.
• Review the tests too. Ask for a test of the failure path. The original pull request had none, which is how the swallowed exception got that far.
• Small pull requests. Every rule above is easier to apply to a 150-line diff than to a 1,500-line one.

Summary

• Name things for the caller, with units where they matter.
• Keep each function at one level of work; describe it in one sentence without "and".
• Handle specific errors with a clear outcome and let the rest propagate.
• Write comments for the why, and delete comments that repeat the code.
• Prefer guard clauses and one straight path over deep nesting.

References: Ruff rules, Python exception handling tutorial, PEP 8 naming conventions.