Design Principles Behind the Code Quality Numbers

The last post started from a number and asked what it means. This post starts from a change request.

The shape of every section is the same. Someone asks for a change. The change is harder than it should be. A design principle explains why. The code is fixed, and one of the nine numbers from the last post changes.

Every number in a code block comes from running a tool on the code, once before the fix and once after.

A year later, the shop

The last post measured one function, calculateDiscount. It belongs to a small online store with five modules. Checkout, pricing, shipping, invoice and email.

A year on, the store is still small, but checkout.js is 85 lines. Every new feature was written into it. It holds:

• the coupon rules and the tax rules
• the shipping price tiers and the invoice text layout
• a direct call to the company that sends the emails
• three currency functions written for a euro launch that never happened

The tests pass. Nobody wants to open the file. Here's what happens when seven ordinary requests come in.

1. The senior age moves to 65

The request. Marketing changes the senior discount from age 60 to 65.

What goes wrong. The rule is written in two places. A year ago the shipping discount was copied from the pricing discount, so both files have the same line.

// pricing.js, before
if (age >= 60) {
  discount += 5;
}

// shipping.js, before
if (age >= 60) {
  discount += 5;
}

The developer changes pricing.js and updates the boundary test. A boundary test checks the values just below and just above a limit, so here it checks 64 and 65. The test fails, because the shipping copy still says 60.

test('64 is not a senior anywhere', () => {
  assert.strictEqual(
    calculateDiscount(64, false, 100) + calculateShippingDiscount(64, false, 100),
    0
  );
});

$ node --test tests/shop.test.js

not ok 3 - 64 is not a senior anywhere
  expected: 0
  actual: 5

The 5 is the shipping discount that a 64-year-old should no longer get. Without that test, customers aged 60 to 64 would keep it for months. Nobody would notice, because nothing would break.

The principle. Don't repeat yourself, DRY. Two authors, Hunt and Thomas, gave it that name in 1999. Every business rule should be written in exactly one place. Two lines that look alike are fine. Two copies of the same rule will go out of step the first time one of them changes.

How to check it. Before you change a rule, search the codebase for the number or the name in it. If it comes up twice, it has two copies.

The fix. One module holds the member rules. Both callers use it.

// discounts.js
const SENIOR_AGE = 65;

function assertValidOrder(age, orderValue) {
  if (age < 0 || orderValue < 0) {
    throw new Error('invalid input');
  }
}

function memberDiscount(age, premiumMember) {
  let discount = 0;
  if (premiumMember) {
    discount += 10;
  }
  if (age >= SENIOR_AGE) {
    discount += 5;
  }
  return discount;
}

// pricing.js
const { assertValidOrder, memberDiscount } = require('./discounts.js');

function calculateDiscount(age, premiumMember, orderValue) {
  assertValidOrder(age, orderValue);
  let discount = memberDiscount(age, premiumMember);
  if (orderValue > 10000) {
    discount += 10;
  }
  return Math.min(discount, 20);
}

The input check moved as well, because both files had a copy of that too. shipping.js now calls the same two functions and keeps only the two things that are really its own, the order value where its bonus starts and its cap.

The number that changed. Duplication. jscpd looks for blocks of code that appear more than once. Same command on the whole folder, before and after.

$ npx jscpd --min-lines 5 .                           # before
Clone found (javascript)
 - pricing.js [1:27 - 12:19] (12 lines, 58 tokens)
   shipping.js [1:35 - 12:19]
Duplicated lines: 11 (6.15%)

$ npx jscpd --min-lines 5 .                           # after
Found 0 clones.

A clone is one block found in two places. The tool counts 11 duplicated lines out of 179 lines in the folder, which is the 6.15%.

2. Add a coupon code

The request. Support two coupons. SAVE10 gives 10% on orders above 1,000. SAVE20 gives 20% on the same orders, premium members only.

What goes wrong. The coupon logic was added the way most logic gets added, as another if inside the one before it.

function applyCoupon(order, discount) {
  if (order.coupon) {
    if (order.coupon.startsWith('SAVE')) {
      if (order.value > 1000) {
        if (order.coupon === 'SAVE10') {
          discount += 10;
        } else if (order.coupon === 'SAVE20') {
          if (order.premium) {
            discount += 20;
          }
        }
      }
    }
  }
  return discount;
}

Five levels deep. Take one question. Does a premium member with SAVE20 and an order of 900 get anything? To answer it you have to remember every condition above that line. The answer is no, because 900 is not above 1,000, and it took four conditions to get there.

The third coupon will go in as a sixth level, because a nested if is the easiest place to add one.

The principle. Keep it simple, KISS. The phrase comes from the US aircraft industry in the 1960s. In code it means a reader can read a function from top to bottom once and understand it.

How to check it. Count the if levels. If you are more than two deep, flatten the function.

The fix. The coupons become a table. The checks become guard clauses. A guard clause is a check at the top of a function that returns at once when the case doesn't apply, so nothing needs to be nested.

// coupons.js
const COUPONS = {
  SAVE10: { minValue: 1000, premiumOnly: false, discount: 10 },
  SAVE20: { minValue: 1000, premiumOnly: true, discount: 20 },
};

function couponDiscount(order) {
  if (!Object.hasOwn(COUPONS, order.coupon)) return 0;
  const coupon = COUPONS[order.coupon];
  if (order.value <= coupon.minValue) return 0;
  if (coupon.premiumOnly && !order.premium) return 0;
  return coupon.discount;
}

The first line uses Object.hasOwn instead of a plain lookup, because coupon codes are typed by customers. Every JavaScript object has a built-in toString function. A customer who types toString as a coupon would get that function back from a plain lookup, and the total would become NaN.

The nested version was safe only because startsWith('SAVE') filtered that out. The new version has a test for it.

test('a coupon named like an object method is not a coupon', () => {
  assert.strictEqual(couponDiscount({ ...base, coupon: 'toString' }), 0);
});

The number that changed. Cognitive complexity, from eslint-plugin-sonarjs, an ESLint plugin that scores how hard a function is to read. Nesting costs extra.

applyCoupon      cyclomatic 7   cognitive 16     # before
couponDiscount   cyclomatic 5   cognitive 4      # after

Cyclomatic complexity, the number of paths, only dropped by two. Cognitive complexity dropped from 16 to 4, because the nesting is gone and nesting is what it charges for.

3. The euro launch that never came

There was no request this time. A year ago someone said the shop might sell in euros, so checkout.js got toEuro, fromEuro and formatEuro, exported and ready.

What goes wrong. Nothing calls them. Every developer who opens the file still reads them, and every metric still counts them.

The mutation tester from the last post shows the cost. Stryker makes small deliberate changes to the code, called mutants, and runs the tests against each one. A mutant is killed when a test fails, and it survives when every test still passes.

$ npx stryker run --mutate checkout.js

checkout.js  |  71.29  |  killed 72  |  survived 29

71.29 is the percentage killed. The 29 survivors, by function:

currency functions   10    no test calls this code
invoiceText           9    changed text nobody checks
applyCoupon           5
calculateTotal        4    the limits > 5000, > 10 and > 2, each changed to >=
checkout              1    the email subject

Ten of the 29 can never be killed, because no test has a reason to call the currency code. The four in calculateTotal are missing boundary tests, and request 6 adds them.

The principle. You aren't going to need it, YAGNI. It comes from Extreme Programming, a way of working from the late 1990s. Write the code when the request arrives. The euro launch did not arrive, and the code was written anyway.

How to check it. For each function, name one caller. If you can't, delete it.

The fix. Delete the three functions and their export. Git keeps the old version if the launch ever happens.

The number that changed. Dead code, from knip, a tool that lists files and exports that nothing imports.

$ npx knip                                            # before
Unused files (1)
rules-engine.js
Unused exports (3)
toEuro      checkout.js:85:59
fromEuro    checkout.js:85:67
formatEuro  checkout.js:85:77

$ npx knip                                            # after
(no output, exit code 0)

knip also found rules-engine.js. Nothing imports that either. It comes back in request 7, so it stays for now.

4. Change the invoice layout

The next three principles are easy to mix up, so here is what each one looks at. Separation of concerns asks which file holds what. Coupling asks how much one module knows about another. Single responsibility asks which team would ask for a change. One request each.

The request. The invoice text needs a new line for the delivery address.

What goes wrong. The layout is written inside checkout.js, next to the coupon rules, the tax rules and the email call.

// checkout.js, before
function invoiceText(order, total) {
  const lines = [];
  lines.push('INVOICE');
  lines.push('-------');
  lines.push('Customer: ' + order.name);
  lines.push('Items:    ' + order.items.join(', '));
  lines.push('Total:    ' + total.toFixed(2));
  return lines.join('\n');
}

A layout change means opening the busiest file in the repository. The change appears in the same file as last week's tax fix. The reviewer has to work out which lines are layout and which lines are pricing.

The principle. Separation of concerns. Dijkstra, a Dutch computer scientist, used the phrase in 1974. A concern is one kind of thing the code deals with. Layout is one. Tax is another. Each concern gets its own place, so a change in one touches one file.

How to check it. Ask whether two different people would request two changes to this file. If a designer changes the invoice and an accountant changes the tax, those are two files.

The fix. The layout moves to its own file.

// invoice-layout.js
function invoiceText(order, total) {
  return [
    'INVOICE',
    '-------',
    'Customer: ' + order.name,
    'Items:    ' + order.items.join(', '),
    'Total:    ' + total.toFixed(2),
  ].join('\n');
}

The delivery address is one more string in that array, in a file that changes only when the invoice changes.

The number that changed. File size, from wc -l, which counts lines. This move alone takes 9 lines out of checkout.js. The other requests take out the rest, and after all seven the work that used to sit in one 85-line file sits in six files.

$ wc -l checkout.js coupons.js tax.js shipping-cost.js invoice-layout.js notify.js
  21 checkout.js
  14 coupons.js
  10 tax.js
  15 shipping-cost.js
  11 invoice-layout.js
   8 notify.js

This is the hotspot from the last post. One file that changes for three different reasons changes three times as often as it should.

5. Switch the email provider

The request. Move from SendGrid, the company whose service sends the shop's emails, to another provider.

What goes wrong. checkout.js loads sendgrid.js with require and calls the vendor's function by name.

// checkout.js, before
const { sendGridSend } = require('./sendgrid.js');

function checkout(order) {
  const total = calculateTotal(order);
  const text = invoiceText(order, total);
  sendGridSend(order.email, 'Your invoice', text);
  return total;
}

So the provider swap means editing the file that holds the pricing logic. In this shop there is one place that sends email. In a bigger shop there would be ten, and every one of them would name the vendor.

The principle. High cohesion, low coupling. Larry Constantine described both in the late 1960s, and a 1979 book, Structured Design, made them standard. Cohesion means everything inside a module is about one idea. Coupling means how much a module knows about the inside of another. A vendor's function name inside the checkout file is high coupling.

How to check it. Coupling: if the vendor renamed its function tomorrow, how many of your files would change? Cohesion: can you name the file in three words with no "and" in them?

The fix. One small module knows the provider. Everything else asks it to send an invoice.

// notify.js
const { sendGridSend } = require('./sendgrid.js');

// The only file that knows which email provider we use.
function sendInvoice(to, text) {
  return sendGridSend(to, 'Your invoice', text);
}

// checkout.js, after
const { sendInvoice } = require('./notify.js');

function checkout(order) {
  const total = calculateTotal(order);
  sendInvoice(order.email, invoiceText(order, total));
  return total;
}

The cohesion half is what requests 2, 4 and 6 do. Coupons, layout and tax each end up in a file that is about one thing.

The number that changed. The import graph, which is the list of which file loads which. madge draws it. Test files are left out.

$ npx madge checkout.js                               # before
checkout.js
  pricing.js
  sendgrid.js
  shipping.js

$ npx madge checkout.js                               # after
checkout.js
  coupons.js
  invoice-layout.js
  notify.js
  pricing.js
  shipping-cost.js
  tax.js
notify.js
  sendgrid.js
pricing.js
  discounts.js
shipping-cost.js
  shipping.js

Before, sendgrid.js was loaded by the 85-line file. After, it's loaded by notify.js only, which is 8 lines long. The provider swap is now a change to that one file.

checkout.js loads six modules instead of three. That's fine. Nothing loads checkout.js except the tests, so in the last post's terms its instability is 1, which means it is free to change. Low coupling is not about the number of imports. It's about whether the importing file knows the inside of what it imports.

6. Add a tax rule per country

The request. Germany's rate changes, and two more countries are coming.

What goes wrong. Tax is computed inside calculateTotal, together with everything else that makes up a price.

function calculateTotal(order) {
  let discount = calculateDiscount(order.age, order.premium, order.value);
  discount = applyCoupon(order, discount);
  let tax;
  if (order.country === 'IN') {
    tax = 18;
  } else if (order.country === 'DE') {
    tax = 19;
  } else if (order.country === 'US') {
    tax = order.state === 'CA' ? 7.25 : 0;
  } else {
    tax = 20;
  }
  let shipping;
  if (order.value > 5000) {
    shipping = 0;
  } else if (order.weightKg > 10) {
    shipping = 250;
  } else if (order.weightKg > 2) {
    shipping = 120;
  } else {
    shipping = 60;
  }
  shipping -= calculateShippingDiscount(order.age, order.premium, order.value);
  if (shipping < 0) {
    shipping = 0;
  }
  const afterDiscount = order.value - (order.value * discount) / 100;
  return afterDiscount + (afterDiscount * tax) / 100 + shipping;
}

It does five things: discount, coupon, tax, shipping tiers and shipping discount. And three different teams ask for changes to it. Marketing changes discounts, finance changes tax, logistics changes shipping. A tax change edits the function that also computes shipping, and every tax test runs through the shipping code.

The principle. Single responsibility. Robert C. Martin wrote it down in 2002. A function or module should have only one reason to change. Reason means the team or person who would ask, not the number of steps in the code. A function can do several steps as long as they all change together.

How to check it. List who would ask for a change to this function. More than one name means more than one function.

The fix. One function per reason. Tax, shipping, and the sum.

// tax.js
const TAX_RATES = { IN: 18, DE: 19, US: 0 };
const DEFAULT_RATE = 20;

function taxRate(order) {
  if (order.country === 'US' && order.state === 'CA') return 7.25;
  if (!Object.hasOwn(TAX_RATES, order.country)) return DEFAULT_RATE;
  return TAX_RATES[order.country];
}

// shipping-cost.js
function shippingCost(order) {
  if (order.value > 5000) return 0;
  if (order.weightKg > 10) return 250;
  if (order.weightKg > 2) return 120;
  return 60;
}

function shippingToPay(order) {
  const discount = calculateShippingDiscount(order.age, order.premium, order.value);
  return Math.max(0, shippingCost(order) - discount);
}

// checkout.js, after
function calculateTotal(order) {
  const discount = calculateDiscount(order.age, order.premium, order.value) + couponDiscount(order);
  const afterDiscount = order.value - (order.value * discount) / 100;
  const tax = (afterDiscount * taxRate(order)) / 100;
  return afterDiscount + tax + shippingToPay(order);
}

Two new countries are two entries in TAX_RATES. The three shipping limits now sit in a four-line function, and the boundary tests that request 3 found missing go next to it.

test('order of exactly 5000 still pays shipping', () => {
  assert.strictEqual(calculateTotal({ ...base, value: 5000 }), 5000 * 1.18 + 60);
});

test('order of 5001 ships free', () => {
  assert.strictEqual(calculateTotal({ ...base, value: 5001 }), 5001 + (5001 * 18) / 100);
});

The number that changed. Cyclomatic complexity, from ESLint.

calculateTotal   cyclomatic 9   cognitive 11     # before

calculateTotal   cyclomatic 1   cognitive 0      # after
taxRate          cyclomatic 4   cognitive 3
shippingCost     cyclomatic 4   cognitive 3
shippingToPay    cyclomatic 1   cognitive 0

The four numbers after add up to 10, one more than before, because the Object.hasOwn check is a new decision. The paths did not disappear. They moved into functions that each have one reason to change, and each one can be tested by itself.

7. "Make the discount rules configurable"

The request. Product, the team that decides what the shop offers, wants to change discount rules without a deploy. A deploy is a release of new code to the live site.

What goes wrong. Last quarter's answer was rules-engine.js. The rules are data, and a general function reads them and applies them.

const DISCOUNT_RULES = [
  { name: 'premium', when: [{ field: 'premium', op: 'truthy' }], then: { add: 10 } },
  { name: 'senior', when: [{ field: 'age', op: 'gte', value: 60 }], then: { add: 5 } },
  { name: 'big order', when: [{ field: 'value', op: 'gt', value: 10000 }], then: { add: 10 } },
  { name: 'cap', when: [], then: { cap: 20 } },
];

function evaluate(rules, context) {
  let result = 0;
  for (const rule of rules) {
    if (matches(rule, context)) {
      if (rule.then.add !== undefined) {
        result += rule.then.add;
      }
      if (rule.then.cap !== undefined && result > rule.then.cap) {
        result = rule.then.cap;
      }
    }
  }
  return result;
}

Plus a matches function that checks each condition and a getField function that reads a field by name. The file is 44 lines. It expresses the same three rules as the 18-line pricing.js from the last post.

Look at the senior rule. It still says 60. Nobody updated it in request 1, because nobody knew this copy existed. The engine was a third copy of the rule, so it is the DRY bug from request 1 with more code around it.

Product never edited the JSON either, because editing a file in the repository is still a deploy. Nothing imports the engine.

The principle. The right level of abstraction. Abstraction means writing code that handles a general case instead of one specific case. A function that handles any coupon is more abstract than one that handles SAVE10. A rule engine that handles any rule is more abstract than three ifs.

More abstraction is not always better. Request 1 had too little: the same rule copied twice, so the next change was another copy. This request has too much: a general engine for three rules, so the next change is a new engine feature. The level is right when the next real request is a small edit.

How to check it. Ask what the third case would cost. If it's a new line, the level is right. If it's a new copy, there is too little abstraction. If it's a new feature in a general mechanism, there is too much.

The fix. Keep the plain function. Delete the engine. If product really needs to change rules between deploys, the answer is a small config file.

// discount-config.json
{ "seniorAge": 65, "premiumDiscount": 10, "seniorDiscount": 5, "bigOrderFrom": 10000 }

The number that changed. Lines and cognitive complexity for the same three rules. The pricing.js row is the version from the last post, before request 1 moved the member rules out.

rules-engine.js   44 lines   evaluate: cyclomatic 6, cognitive 10   0 callers
pricing.js        18 lines   calculateDiscount: cyclomatic 6, cognitive 4

Same paths and same rules, in less than half the code.

When the principles disagree

The seven don't always point the same way. Four cases, and how to decide each one.

• DRY or KISS. Pulling every repeated line into a shared function makes the code shorter and harder to read. The rule of three, from Fowler's book Refactoring: write it, copy it once, and on the third copy pull it out, when you can see what the three have in common. That is for code that only looks alike. A business rule is one fact, so it gets one place at the second copy, which is why request 1 didn't wait. The test: if the two copies changed for different reasons, would that be a bug? If yes, it's a rule.
• YAGNI or separation of concerns. Splitting one file into six before there is a second reason to change is work for a request that hasn't come. Split when the second reason arrives. In the shop that was the day the layout change and the tax fix appeared in the same diff.
• Abstraction or KISS. A rule engine is more general than three ifs and harder to read than three ifs. Pay for generality only when the rules change more often than the code is released.
• Single responsibility or DRY. Two functions that look the same today may change for different reasons tomorrow. The pricing cap and the shipping cap have the same shape, and marketing owns one while logistics owns the other. Sharing the member rules was right. Sharing the caps would have been wrong.

The map back to the numbers

Each principle has a number from the last post that warns you before the request arrives.

And seven questions for any pull request, one per principle:

1. Does this rule already exist somewhere else?
2. Is any if more than two levels deep?
3. Is every function this adds called by something?
4. Would a designer's change and an accountant's change touch the same file?
5. Does any file name a vendor it doesn't need to know?
6. How many teams would ask for changes to the biggest function here?
7. If a third case came tomorrow, would it be a new line, a new copy, or a new feature?

For code written by an AI model

A coding model breaks two of these first. It writes a second copy of a rule rather than finding the first, so DRY goes first. And it writes helpers for cases nobody asked for, so YAGNI goes next.

The last post ended with a prompt for coding models. Two lines to add to it, in the section that lists what to do before writing code:

- Before adding a rule, search for where that rule already lives. Change it there.
- After the task, list every function you added and its callers. Delete any with none.

The other five need a reviewer, whether the code came from a model or a person.

Where this leaves the shop

Seven requests and seven fixes. The tests were run one last time on the finished code.

$ node --test tests/shop.test.js
# pass 22
# fail 0

checkout.js is 21 lines. Every rule is written once. Each time, a number from the last post said which principle to use, and this post is those seven cases.