Great Craft in the Smallest Details

During a code review today, I came across a piece of code like this:

def update_balance(input_value) do
  ...
  commission_rate = get_config("comission_rate")
  balance1 = data1.balance + input_value * (1 - commission_rate)
  ...
  balance2 = data2.balance + input_value * commission_rate
  ...
end

To make the discussion easier, I’ve tweaked the variable names a little and stripped out the unnecessary details. At first glance the code looks fine: the incoming input_value is split into two parts — one part is the commission on this transaction, credited to balance2, and the remainder is credited to balance1.

Think about it carefully, though, and there are two big problems:

1. It violates the Open-Closed Principle (OCP)
2. It isn’t safe

Let’s take them one at a time.

The Open-Closed Principle

The Open-Closed Principle says that software entities should be open for extension, but closed for modification. It comes out of OOP, but we can generalize it well beyond OOP.

In the code above, computing the transaction commission and update_balance are independent concerns. update_balance doesn’t need to know — and shouldn’t know — the details of how commission is calculated in order to do its own job. The reason it shouldn’t know is this: once update_balance knows that detail, any change in how commission is computed necessarily drags update_balance along with it, and since that knowledge is scattered across different points in the function, one change tugs at everything.

So what kinds of changes might occur? That depends on the programmer’s product sense. commission_rate is a rate. Since commission can be taken as a percentage of a transaction, might it one day be a flat fee instead? Or a flat fee below some transaction size and a percentage above it? Or something more elaborate still — tiered, progressive rates?

So we should pull the calculation out into its own function, say calc_commission, and update_balance becomes:

def update_balance(input_value) do
  ...
  commission = calc_commission(input_value)
  balance1 = data1.balance + input_value - commission
  ...
  balance2 = data2.balance + commission
  ...
end

def calc_commission(input_value) do
  commission_rate = get_config("commission_rate")
  round(input_value * commission_rate)
end

This way, however the commission calculation changes, update_balance adapts naturally. That is what it means to be open for extension and closed for modification.

Code Safety

Writing code is a serious and rigorous undertaking, yet constrained by the programmer’s own state of mind and by circumstance — whether you can write without being interrupted or distracted, whether the project is on fire, whether the boss is hounding you every day — code all too easily gets written carelessly and unsafely. From a safety standpoint the code above looks harmless; but think again: a bad commission_rate can drive the result in exactly the opposite direction of intent — say, a commission_rate greater than 1. You might argue that we can post a guard at the system’s border, validate commission_rate as it enters, and refuse to let non-conforming data into the system. Most of the time that strategy is fine, but in important settings, layered defenses are necessary. A hacker may have carefully crafted data that overflows a buffer and knocks out the guard at the border, defeating the check; more likely still, this code later gets reused in another system that provides no such border check, and the attacker walks right in.

Most problems like the ones above come from an imprecise domain and range for the function. A programmer who hasn’t trained that instinct makes mistakes easily; and of course programming languages don’t offer good tools for pinning down a function’s domain and range — even something as powerful as Haskell can’t easily constrain a type to a particular range of values. In the OCP section we extracted calc_commission; for this version of the function, no matter how commission_rate varies, the computed commission should fall within [0, input_value]. So calc_commission should be:

$f(rate)=\begin{cases} 0, & \text{if rate > 1} \ input_value, & \text{if rate < 0} \ input_value * rate \end{cases}$

and the corresponding code:

def calc_commission(input_value) do
  commission_rate = get_config("commission_rate")
  max(0, min(input_value, round(input_value * commission_rate)))
end

Here min/max lock the function’s range to [0, input_value]. If commission_rate takes an invalid value, commission is at most input_value and at least 0, which eliminates the risk of overflow.

A good habit worth cultivating here: write down the mathematical formula for the key functions you write. A formula is rigorous by nature; in expressing one, you won’t leave out the unreasonable members of the domain, and the function’s range thus acquires a rigorous bound.

The Sage’s Moment

Zengzi lay gravely ill in bed. Yue-zheng Zichun sat beneath the bed; Zeng Yuan and Zeng Shen sat at his feet; a young attendant sat in the corner holding a candle. The boy said, “So brightly figured and so smooth — is this not the mat of a minister?” Zichun said, “Hush!” Zengzi heard it, started up, and cried, “Ah!” The boy said again, “So brightly figured and so smooth — is this not the mat of a minister?” Zengzi said, “It is. It was a gift from Ji Sun, and I have not had the strength to change it. Yuan, raise me up and change the mat.” Zeng Yuan said, “The Master’s illness is critical; he must not be moved. If only he lasts until dawn, I will respectfully change it then.” Zengzi said, “Your love for me is not the equal of his. The gentleman loves others through virtue; the petty man loves others through indulgence. What more could I ask for? To die having got it right — that is enough.” They lifted him up and changed the mat, and before he was settled back on it, he passed away.

Zengzi lay sick in bed, his condition grave. Yue-zheng Zichun sat below the bed, Zeng Yuan and Zeng Shen sat by his feet, and a young servant sat in the corner holding a candle. The servant said, “This mat is so ornately figured and smooth — isn’t it the mat of a minister?” Yue-zheng Zichun said, “Silence!” Zengzi heard it and, suddenly roused, said, “Ah!” The servant said again, “The mat is ornately figured and smooth — isn’t it the mat of a minister?” Zengzi said, “Yes. It was a gift from Ji Sun, and I have not had the strength to change it. Yuan, help me up and change the mat.” Zeng Yuan said, “Your illness is already critical, you cannot be moved; I beg you to wait until dawn, and then let me change it.” Zengzi said, “Your love for me is not the love that boy bears me. The gentleman loves others through virtue; the petty man loves others through indulgence. What do I still ask for now? I only hope to die in accordance with the rites.” So everyone lifted Zengzi up and changed the mat, then set him back on the bed, and before he was settled, Zengzi passed away.

This is the famous story of “Zengzi changing his mat.” By the rites of Zhou, Zengzi had never held office as a minister (大夫) and was therefore not entitled to use a mat reserved for ministers. It was entirely unintentional, to be sure, but if he had died on a minister’s mat, that would have been a breach of propriety; and so, even at the point of death, he still ordered his son to change it. Read pedantically, one could call Zengzi a fool and say he is not worth imitating — but the story itself carries the message “do not commit a small wrong” and “a thousand-li dike is brought down by an ant hole.” To indulge is “the love of a petty man,” and it erodes virtue. Two or three small lines of code, left unexamined, can likewise make an entire system fragile.