DEV Community

Revin
Revin

Posted on Originally published at revin.com.br

1,043 branches in a pricing service: 34 changed last year, 9 disagree with the spreadsheet

Someone gave me read access to a pricing service last month with a question I could not answer by opinion: should we buy a rules engine? Node and TypeScript, roughly 40k lines, six files under src/pricing, three developers in its history and one of them still around.

The vendor deck said hundreds of rules. The team said thousands. Nobody had counted, and the count was never the interesting number anyway. What decides that purchase is how many of those rules move, how often, and who has to be in the room when they do.

So I spent an afternoon measuring three things: how many branches exist, how many of them changed in the last twelve months, and how many of the same rules also live in the finance spreadsheet with a different value. The third number was the one that mattered, and it was the only one nobody had asked for.

Counting branches, and the first attempt that lied to me

My first pass was the lazy one:

$ rg -c '\bif\s*\(' src/pricing
src/pricing/commission.ts:274
src/pricing/discount.ts:141
src/pricing/creditLimit.ts:98
src/pricing/tax.ts:87
src/pricing/legacyPartner.ts:61
src/pricing/shipping.ts:28
Enter fullscreen mode Exit fullscreen mode

689 total, and wrong in both directions. It counts if inside comments and string literals, and it misses every ternary and every case. In this codebase the ternaries were not decoration, half the tier selection was written as chained ? :.

A parse gives an honest number:

// count-branches.js
const { Project, SyntaxKind } = require('ts-morph')

const project = new Project({ tsConfigFilePath: './tsconfig.json' })
const kinds = [
  SyntaxKind.IfStatement,
  SyntaxKind.ConditionalExpression,
  SyntaxKind.CaseClause
]

for (const file of project.getSourceFiles('src/pricing/**/*.ts')) {
  const total = kinds.reduce(
    (sum, kind) => sum + file.getDescendantsOfKind(kind).length,
    0
  )
  if (total > 0) console.log(total, file.getFilePath())
}
Enter fullscreen mode Exit fullscreen mode
$ node count-branches.js | sort -rn
412 src/pricing/commission.ts
196 src/pricing/discount.ts
143 src/pricing/creditLimit.ts
121 src/pricing/tax.ts
116 src/pricing/legacyPartner.ts
 55 src/pricing/shipping.ts

$ node count-branches.js | awk '{ s += $1 } END { print s }'
1043
Enter fullscreen mode Exit fullscreen mode

1,043 decision points. Big enough to scare anyone in a planning meeting, and useless on its own.

Which of them actually moved

File level granularity is crude, and it was enough to change the conversation:

$ for f in $(git ls-files 'src/pricing/*.ts'); do
    printf "%3d %s\n" "$(git log --since='12 months ago' --oneline -- "$f" | wc -l)" "$f"
  done | sort -rn
 31 src/pricing/commission.ts
 12 src/pricing/discount.ts
  4 src/pricing/creditLimit.ts
  0 src/pricing/tax.ts
  0 src/pricing/legacyPartner.ts
  0 src/pricing/shipping.ts
Enter fullscreen mode Exit fullscreen mode

Drop the commits that only touched imports or formatting and 47 becomes 34 real rule changes in a year, 31 of them in one file. Three files with 292 branches between them had not been touched since 2021.

For the hot file I narrowed it down with git log -L, which follows a function across renames well enough:

$ git log -L '/function calculateCommission/',+60:src/pricing/commission.ts \
    --since='12 months ago' --format='%h %ad %an' --date=short | grep -c '^[0-9a-f]\{7\}'
19
Enter fullscreen mode Exit fullscreen mode

Nineteen changes to one function in twelve months, and every one of them went through a ticket, a review and a deploy. That is the real argument for pulling something out of the code, and it applies to about 3% of the branches in that repo. The other 97% are fine where they are.

The 0.125 nobody could explain

While I was in there, I picked the strangest constant and asked git who put it there:

$ git log -S'0.125' --oneline --date=short --format='%h %ad %an %s' -- src/pricing/commission.ts
a3f19c2 2019-11-08 <redacted> fix commission for partner channel
Enter fullscreen mode Exit fullscreen mode

One commit, one line of message, author no longer at the company. The diff shows what happened. Nothing anywhere shows why the partner channel got half a point more, or whether that was meant to expire.

Version control is the best record of what was done that most companies own, and a poor record of why it was done. A rules engine does not fix that either. It just moves the undocumented number to a different screen.

The spreadsheet nobody put in the architecture diagram

Then I asked for the workbook finance uses to close the month. Formula cells are business rules running in production without tests, history or an owner, so I counted them:

# formulas.py
import re
from openpyxl import load_workbook

wb = load_workbook("month-close.xlsx")
rate = re.compile(r"0\.\d+|\d+(?:\.\d+)?%")

for ws in wb.worksheets:
    for row in ws.iter_rows():
        for cell in row:
            value = cell.value
            if isinstance(value, str) and value.startswith("="):
                found = rate.findall(value)
                if found:
                    print(ws.title, cell.coordinate, found, value[:64])
Enter fullscreen mode Exit fullscreen mode
$ python formulas.py | tee sheet-rates.txt | head -4
Commissions D14 ['0.125'] =IF(C14>45000,B14*0.125,B14*0.1)
Commissions D15 ['0.125'] =IF(C15>45000,B15*0.125,B15*0.1)
Discounts   H8  ['0.08']  =IF(AND(F8>50,G8<>"distributor"),E8*0.08,E8*0.05)
Discounts   H9  ['0.05']  =IF(AND(F9>50,G9<>"distributor"),E9*0.08,E9*0.05)

$ python formulas.py | wc -l
148
$ python formulas.py | grep -o "\['[^]]*\]" | sort -u | wc -l
23
Enter fullscreen mode Exit fullscreen mode

148 formula cells, 23 distinct rates. I pulled the numeric literals out of the six pricing files with the same AST script and compared the two lists. Fourteen matched. Nine did not.

The first line of that output is the expensive one. The service pays the higher commission tier above 50,000 and the spreadsheet pays it above 45,000. Same rate, different threshold. Invoices come out of the system, commissions get paid from the sheet, and the gap only shows up when a rep notices their own number.

That comparison took about forty minutes to write and it is the measurement I would run first if I did this again, before counting a single branch.

The table that turned into an interpreter

The obvious move is to pull the rates out of the code and into data. It works, right up to the point where it does not.

create table commission_rule (
  id            serial primary key,
  channel       text not null,
  min_amount    numeric not null,
  max_amount    numeric,
  rate          numeric not null,
  effective_from date not null,
  owner         text not null
);
Enter fullscreen mode Exit fullscreen mode

Tiers, caps and thresholds fit here without complaint, and most requests from the business are exactly that: change a number. Then I hit this one, which exists in that codebase almost word for word:

// 8% off above 50 units, except distributors, who get 5%,
// unless the contract predates 2023
const rate =
  order.units > 50
    ? order.customerType === 'distributor'
      ? contractSignedBefore(order, '2023-01-01')
        ? 0.08
        : 0.05
      : 0.08
    : 0
Enter fullscreen mode Exit fullscreen mode

To express that in columns I added customer_type, then contract_signed_before, then a priority column, then a first match wins evaluator to read the priority. Two hours in, I was writing a rules engine inside the product, with no debugger, no tests and exactly one person who would ever understand it. I threw it away.

Where the line ended up

Numbers go to the table, with an owner and an effective date. Chained conditions stay in the code, with a test whose name spells out the business case:

test('distributor keeps 5% above 50 units when the contract predates 2023', () => {
  const order = {
    units: 120,
    customerType: 'distributor',
    contractSignedAt: '2022-04-19'
  }

  expect(discountRateFor(order)).toBe(0.05)
})
Enter fullscreen mode Exit fullscreen mode

23 constants left the code. The nine mismatches got reconciled against the sheet, which was a finance conversation and not an engineering one. The chained rules stayed exactly where they were, and nobody has asked to change them since.

On the original question: 34 changes a year, concentrated in one file, with no separate business function operating the rules and no auditor asking who changed what. Buying an engine there would have added an environment to run, test and roll back, in exchange for removing a deploy that happens about three times a month.

Two things I still do not have a good answer for. The AST comparison between spreadsheet constants and code constants was a throwaway script, and I would like it running in CI, but matching a formula to the function that implements it is fuzzy work and I stopped at eyeballing 23 lines. And I have no idea where this line sits for a team of two with twenty rules, where the whole exercise probably costs more than the problem.

How do you keep rate constants from forking between the system and whatever the business uses to close the month? Has anyone automated that diff in a way that survives a column being renamed?

Originally published on the Revin blog.

Top comments (0)