Writing Less Error-Prone Code

Oli Peate

How much time do you spend worrying about avoiding duplication in code you’ve written? And how about when you review a pull request? Contrast that commitment to this follow-up question:

How much time do you spend thinking about making code less susceptible to errors in the future?

A line of code you write in a codebase under active development may be read tens, hundreds or thousands of times. This line is likely to be adapted, moved and copied multiple times. How should we optimize for this reality of a living codebase?

Daydream

Picture the scene: You’ve written well-tested code which completes a feature. The resulting pull request is concise and clear; it’s approved swiftly. The sun in shining. It’s time for lunch… perhaps today is a good day to try the new burrito place?

Eight weeks later a colleague copies a test you wrote originally as the basis for a new test. Regrettably a mistake is introduced — the freshly copied test is missing an after hook to reset a global value after the test. However in isolation the new test passes, as does the whole test suite during the review process.

Huh. A build failed on the CI server.

It’s nine weeks after you wrote the code which was subsequently copied. You’re late for a meeting so you hit the re-build button and dash off. You promise yourself to keep an eye out for that test failing again.

Fast-forward twelve months. The test suite only passes on CI about 60% of the time. Team mates grumble about writing tests — what’s the point anyway if they keep flickering? You feel stressed.

I retract the previous paragraph. This isn’t a horror story. A week after the first re-build you noticed the test failing again. You use the failing seed numbers to determine the combination and order of tests which cause the failure and a fix is applied. Phew.

Side Effects

Seemingly inconsequential decisions have potential side effects. Consider the ramifications from removing the duplicate after hooks within each context:

RSpec.describe "holiday promotions" do
+ after { Timecop.return }

  context "on Independence day" do
    before { Timecop.freeze Date.parse("2017-07-04") }
-   after { Timecop.return }

    it "displays the fireworks banner" do
      # ...
    end
  end

  context "on Thanksgiving" do
    before { Timecop.freeze Date.parse("2017-11-23") }
-   after { Timecop.return }

    it "displays the turkey banner" do
      # ...
    end
  end

  context "on Black Friday" do
    before { Timecop.freeze Date.parse("2017-11-24") }
-   after { Timecop.return }

    it "displays the TV banner" do
      # ...
    end
  end
end

This change has an immediate trade-off on portability. One can no longer copy a given context and re-use it in a new test file without causing time to be left in an incorrect state.

While the original test file has few lines there’s a solid chance the reader will notice the after block and copy it across into the new test file. If the original test file grows to hundreds of lines then the chances of missing the after hook are greater.

For some it might feel instinctive to blame the individual copying the code for not fully reading the original test. Or blame them for not knowing one should always use Timecop.return after freezing time. But then again perhaps the pull request reviewer should share some of the blame too.

Nope. None of these directions are valid or productive thoughts.

There’s another approach. And it’s not setting up a global after hook to ensure time is always reset. That’s bloat. Plus it fixes an instance of this kind of bug. Another variant will occur.

Making Seatbelts Mandatory

How can we make code from the example above less susceptible to errors when it’s re-used? A solution to this particular problem is using a block syntax:

RSpec.describe "holiday promotions" do
  context "on Independence day" do
    it "displays the fireworks banner" do
      independence_day = Date.parse("2017-07-04")

      Timecop.freeze(independence_day) do
        visit(homepage_path)
      end

      expect(page).to have_content "50% off fireworks"
    end
  end

  context "on Thanksgiving" do
    it "displays the turkey banner" do
      thanksgiving = Date.parse("2017-11-23")

      Timecop.freeze(thanksgiving) do
        visit(homepage_path)
      end

      expect(page).to have_content "50% off turkeys"
    end
  end

  context "on Black Friday" do
    it "displays the TV banner" do
      black_friday = Date.parse("2017-11-24")

      Timecop.freeze(black_friday) do
        visit(homepage_path)
      end

      expect(page).to have_content "50% off TVs"
    end
  end
end

Using the block syntax ensures time is put back the way it was. Now there’s no risk of an after hook being omitted. The Timecop library used in these examples lets you opt-in to a ‘safe mode’ which only permits this block syntax.

When writing higher level tests, like those above, our minds need to be focused on the end-user. Ideally we’re free from the distracting details of how time stubbing is implemented. Providing higher levels of abstraction and safety aren’t ‘dumbing down’ a codebase or dulling tools - it’s friendlier and practical. Every contributor gains when we find the right balance between expressiveness and control for the task at hand.

A team working together on a codebase could establish a broader rule building upon this solution: When code modifies global state in a test, prefer blocks to ensure that global state is restored.

This ‘do-then-tidyup’ block syntax pattern is found throughout the Ruby standard library too, from temporarily changing directories with Dir.chdir to closing a file after opening it with File.open.

Readability

Another key aspect to avoiding bugs arising from code ‘after it was born’ (a.k.a the majority of its life) is readability. If code can be read and understood more readily then there’s less opportunity for misunderstandings and mistakes. Consider these two snippets of code:

is_friday? ? display_burger_deal(:cheeseburger) : display_coffee_deal(:flat_white)

if is_friday?
  display_burger_deal(:cheeseburger)
else
  display_coffee_deal(:flat_white)
end

Many developers are familiar with ternary statements. I’d wager familiarity with the concept doesn’t make it much easier for the reader’s brain to digest what’s going on. Especially if they didn’t write the code and have no context to lean on. This example deliberately places a predicate method (is_friday?) next to the ternary operator (?) for maximum brain-parsing awkwardness.

What’s not to like about the second snippet? One might claim it’s ‘wasteful’ because it takes five lines to express what could be achieved in one. I think this response stems from that initial thrill of learning about ternary statements and feeling smart…

WOW this is AWESOME. Hold on a moment while I use this EVERYWHERE.

I’ve felt that thrill too. Don’t give into the smug conciseness of the the one-line predicate. Be proud of emphasizing code branches — at the end of the day comprehension matters.

Communicating With Code

Every line of code is an opportunity to communicate with the reader and demonstrate a style to replicate. Code is more than a set of instructions to a machine. We need to meet additional goals - for humans - like making the code easy to change and easy to grok, especially when our minds are preoccupied with the task at hand.

Begin the journey towards these goals by embracing:

  • Easy to understand code. In practice this means emphasizing readability over outright conciseness.
  • Portability and ‘copy & paste’ reusability. Being able to cut / copy and paste a chunk of code without loss of correctness or meaning is underrated. This form of code reuse is seldom acknowledged but seems to happen all the time, within a codebase or between them.

Strive to write code which is infectiously good; it’s readable, reusable, and adaptable. At any point this code may need to be changed or called upon to act as a kind of template — either extracted into another codebase or remixed within the same codebase to address a whole new challenge.