The Importance of Being Stupid, A Quick Lesson for Clever People

I’ve been thinking lately about how I write code, particularly how as I’ve learnt more tricks about the tools I am using, the cleverer my code gets. This is a quick demonstration of how cleverness obfuscates clarity, and how being stupid is sometimes the smart thing to do.

BDD is generally not my cup of tea; something about the pretence that anyone other than technically inclined people will ever read the tests bothers me, and to my eyes the tests are less clear than a declarative xUnit style.

BDD frameworks, like RSpec, let you do some very clever things, abstracting details, allowing just-in-time creation of objects, overriding behaviour through inheritance, and using natural language.

For example, a (very contrived) RSpec test:

describe 'Users' do
  let(:response) { get_user(id) }                   # 7
  subject(:user) { response.user }                  # 6

  let(:worthing) { FactoryGirl.create(:user, id: 1, name: 'Jack Worthing') }  # 3
  let(:moncrieff) { FactoryGirl.create(:user, id: 2, name: 'Algernon Moncrieff') }

  before do
    generate_given_names(worthing, moncrieff)       # 2

  context 'whilst in London' do
    context 'the given_name' do
      subject(:given_name) { user.given_name_london }      # 5

      context 'for worthing' do
        let!(:id) { 1 }                             # 1
        it 'should be earnest' do
          expect(given_name).to eq 'earnest'        # 4

      context 'for moncrieff' do
        let!(:id) { 2 }
        it 'should be moncrieff' do
          expect(given_name).to eq 'moncrieff'

At first sight it looks quite reasonable until you look at the flow of control here (see the numbered comments). The mix of individually innocuous features turn this into a tangled mess.

Why is this bad?

As the test file grows larger, examples get added. It would be reasonable to assume that the worthing user would be re-usable later on, after all it is defined at the top of the context tree.

The flow of control makes this hard to reason about, following what the state of the world is for any given test can be tricky. One way I’ve often tried to deal with this is to just redefine everything, creating my own little world for each new addition to the test file. Ew.

Context inheritance leads to code that references variables defined physically far away; this is hard for me to read as well, I have to scroll up to the top of the file to check the status of something, before jumping back to where I was.

Poorly optimised behaviour in before filters or let!. In the example above, generate_given_names runs for every single test, even though it returns the same results every time. There’s no potential for re-use. It would be worse if we added more tests where we didn’t need that behaviour and it would be running anyway.

A stupider way?

Here’s an example in MiniTest; we could of course have used RSpec to achieve the same result, by removing a bunch of the features in the example above.

class TestUsers < MiniTest::Unit::TestCase
  def setup
    FactoryGirl.create(:user, id: 1, name: 'Jack Worthing')         # 1
    FactoryGirl.create(:user, id: 2, name: 'Algernon Moncrieff')

  def test_given_names_whilst_in_london
    worthing = get_user(1).user                                     # 2
    moncrieff = get_user(2).user
    generate_given_names(w, m)
    assert_equal(worthing.given_name_london, 'earnest')
    assert_equal(moncrieff.given_name_london, 'moncrieff')

The individual test function is larger, and there will be repetition in further tests. We could clean that up by extracting a function that gets the users and generates the given names. However, I think the flow of control is obvious and unsurprising. Save the cleverness to make your architecture simple, your design elegant. Don’t write clever code when you can write stupid code.