Single responsibility principle

Invoice.ts was edited 31 times last quarter. Finance changed how GST is rounded. Design changed the invoice layout twice. The platform team moved the table from MySQL to Postgres. Three teams, one file, a merge conflict most weeks. Then a layout change shipped with a tax bug in it, because the person fixing the header also “tidied” a line in the total that looked unused.

The file was not too long. It was answering to too many people.

The idea

The usual wording is “a class should have only one reason to change”. That is easy to recite and hard to use, because what counts as a reason? Robert C. Martin’s later wording is the one you can act on: a class should be responsible to one actor. An actor is a person or team who asks for changes.

Before: three bosses

type Item = { name: string; price: number; quantity: number }
 
class Invoice {
  constructor(private readonly items: Item[]) {}
 
  // Finance decides this.
  total(): number {
    const subtotal = this.items.reduce(
      (sum, item) => sum + item.price * item.quantity,
      0,
    )
    return subtotal + subtotal * 0.05
  }
 
  // Design decides this.
  toHtml(): string {
    return `<h1>Invoice</h1><p>Total: ${this.total()}</p>`
  }
 
  // The platform team decides this.
  save(): string {
    return `INSERT INTO invoices (total) VALUES (${this.total()})`
  }
}
 
const items: Item[] = [{ name: 'Masala dosa', price: 120, quantity: 2 }]
const invoice = new Invoice(items)
 
console.log(invoice.total()) // 252
console.log(invoice.save()) // INSERT INTO invoices (total) VALUES (252)

It works. The problem is not visible in the code, it is visible in the git log. Every change by any of the three teams opens this file, re-tests this file, and can break the other two teams’ parts of it.

After: one boss each

type Item = { name: string; price: number; quantity: number }
 
// Finance.
class Invoice {
  constructor(private readonly items: readonly Item[]) {}
 
  total(): number {
    const subtotal = this.items.reduce(
      (sum, item) => sum + item.price * item.quantity,
      0,
    )
    return subtotal + subtotal * 0.05
  }
}
 
// Design.
function renderInvoiceHtml(invoice: Invoice): string {
  return `<h1>Invoice</h1><p>Total: ${invoice.total()}</p>`
}
 
// Platform.
class InvoiceRepository {
  save(invoice: Invoice): string {
    return `INSERT INTO invoices (total) VALUES (${invoice.total()})`
  }
}
 
const items: Item[] = [{ name: 'Masala dosa', price: 120, quantity: 2 }]
const invoice = new Invoice(items)
const repository = new InvoiceRepository()
 
console.log(renderInvoiceHtml(invoice)) // <h1>Invoice</h1><p>Total: 252</p>
console.log(repository.save(invoice))
// > INSERT INTO invoices (total) VALUES (252)

Same behaviour, three places. A layout change now opens one function. The move to Postgres opens one class. Nobody tidying the header can reach the tax line.

Two details are worth a look. The renderer is a plain function, because it has no state to protect. The repository is a class, because the real one would hold a database connection. And readonly Item[] means Invoice promises not to change the array it was given.

How to spot it

Three tests, in the order I use them.

Say what the class does without using “and”. “It calculates the total and renders HTML and saves to the database” is three jobs. “It knows what the customer owes” is one.

List who would ask for a change. If you write down two job titles, you have found the seam. This is the test that works when the first one is ambiguous.

Look at the imports. A class that imports a tax library, a template engine and a database driver is three things glued together. The import list does not lie.

Getting it wrong in the other direction

Checkpoint