Laying Landmines by Being Helpful

In just the last few days, my code reviews have turned up two landmines about to be planted in our code base. Neither would have been horrible, but small mistakes like these will accumulate over the lifetime of an application to make the code impossible to navigate.

I’ll just share the first one to give you the idea. It involved an object that encapsulated an email recipient and whether we wanted to skip sending anything to him for the moment. Excerpted from a larger function, it went like this:

function EmailRecipient(emailAddress, skip) {
    this.emailAddress = emailAddress;
    this.skip = skip;
    this.send = !skip;

The developer thought that some consumers of this class would want to use a skip property but others would find its opposite, send, more natural, so he helpfully provided both names.

But what if a consumer of the class does this?

var recip = new EmailRecipient('', true /*skip*/);

    // ...and then some lines later:
    recip.send = true; // We don't want to skip after all 

    // ...and then still later:
    if (recip.skip) {
        // Do something

After setting recip.send = true, it would be natural to expect recip.skip to be false but of course that’s not the case.

What was intended as a convenience has become a landmine.

Unlike landmines of war, landmines in code are often planted inadvertently and with the best of intentions. Beware!

Leave a Reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.