The sixty-million-dollar mistake

The sixty-million-dollar mistake

Consider the following Java code :-

public class MoneyAmount {

    private double value;
    private String currency;

    public void getValue() {
        return value;
    }

    public void setValue(double value) {
        this.value = value;
    }

    public void getCurrency() {
        return currency;
    }

    public void setCurrency(String currency) {
        String validCcy = CurrencyManager.lookupCurrency(currency);
        if (validCcy != null) {
            this.currency = validCcy;
        }
    }
}

public static void main(String[] args) {
    MoneyAmount amt = new MoneyAmount();
    for (row : spreadsheet) {
        amt.setValue(row.getValue());
        amt.setCurrency(row.getCurrency());
        bookCash(amt);
    }
}

So far so good. All the unit tests pass, let’s deploy it into
production.

Everything goes well for many years. Then a small error occurs in the
input data :-

40000 USD
60000000 JPy

What happens?

Ouch! We’re going to book 60,000,000 USD instead of 60,000,000
Yen. That’s a significant mistake.

You may think that I’ve contrived this example (I couldn’t possibly
confirm or deny that on this blog) but it does show how subtle a
horrifying coding mistake can be.

Why did we make that $60M mistake?

  1. The developer shouldn’t have re-used the instance of MoneyAmount.
  2. The developer should have checked for the null case returning from CurrencyManager.lookupCurrency.
  3. The developer should have avoided using setter methods.

Let’s direct our questioning at a deeper level.

Why did the developer make these mistakes?

  1. The developer didn’t receive adequate education.
  2. Due to tight deadlines, the developer was not given enough time to think about the edge cases.
  3. The developer’s code wasn’t properly reviewed by a code-reviewer.
  4. There weren’t enough tests in the test suite.

These are all potential causes, but it’s not easy to see which one is
relevant and take corrective action.

  1. What constitutes ‘adequate’ education for developers?
  2. How do we manage projects so that estimates are accurate and there are no slippages.
  3. How do we enforce proper code-review and how much review is necessary?
  4. What constitutes adequate testing?

Many people are involved in discovering better answers to these
questions working within, as well as supplementing, their existing
toolsets.

I think these approaches are mostly ineffectual, at some
level. Otherwise I wouldn’t have contemplated bringing in a new
programming language into the bank. In the case of Clojure, it has meant
building a whole new toolset around it (including the IDE where most of
the developers on my team have moved to Emacs).

Mere tools?

Despite the disruption, I’ve been very encouraged by the effectiveness
that a modern tool like Clojure has on software quality. While I
consider a programming language as a tool, that may unfairly diminish
its importance. As an industry we are continually discovering better
ways of creating software and new programming languages are still the
main vessels in which we distill that experience
. Those who adopt a
modern programming language inherit the wealth of a generation’s
experience.

I began this article by presenting a real coding mistake. My experience
with Clojure means that nowadays I approach the issue differently. Why
did the developer make these mistakes?

  1. The tool encouraged it.

Ultimately, delivering reliable software to our users is more important
than our development tools, however much we’ve grown used to them. If
our tools are letting us make these expensive mistakes then we need to
consider their replacement.

5 thoughts on “The sixty-million-dollar mistake

  1. A great example against forcing object-oriented-like style at all costs. The only thing I find hard to believe that it took several years before the application received faulty data for the first time.

  2. A good example of bad OO design. MoneyAmount should have value semantics, rather than reference semantics – so the fields should be final, with values given as arguments to the constructor. It’s preferable not to do things that can fail in a constructor – so the validation of the currency code should be done before constructing the MoneyAmount. Using a static method for the currency lookup prevents you from substituting a different currency manager for test purposes. Also, the currency lookup is hard to understand – if the given string is valid will the same string be returned, or something different? Why not test validity (boolean) rather than returning a string? And the null check in setCurrency, which doesn’t do anything for you when you are dealing with a new MoneyAmount (it simply prevents assigning null to an already-null field), is the kiss of death in your example. If you have to have a setter like this, it should throw an IllegalArgumentException if the given value is invalid.

  3. While this is clearly an example of bad code, it isn’t really Java’s fault. The fundamental mistake is not throwing an exception when asked to use an invalid currency and silently making no change to the data (i.e. fail-fast principle was not followed).

    You could just as easily make the same mistake in idiomatic, pure functional Clojure with something like:
    (if (valid-currency currency) currency old-currency)

  4. Could I suggest one more mistake that I should be made explicit?

    4) the developer shouldn’t use floating point to represent monetary funds, use BigDecimal instead.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>