Is Duplication Always a Bad Thing?

CC image by leg0fenris (original – https://flic.kr/p/8D9EJH)

tl;dr

No. Well, perhaps, but it can be the lesser of two evils. It depends.

Full version

As a software engineer, it’s always been drilled into my head that duplication is a bad thing, and teams I’ve been a part of have always worked diligently to reduce all forms of duplication they see. This as a general rule makes a lot of sense, I can still remember reading through Martin Fowler’s Refactoring while I was at university and the excitement of discovering design patterns.

Duplication is bad

Good developers are precise and careful and understand the risks and pitfalls of duplication in a code base (see the DRY principle, from the pragmatic programmer)

“The alternative is to have the same thing expressed in two or more places. If you change one, you have to remember to change the others, or your program will be brought to its knees by a contradiction. It isn’t a question of whether you’ll remember: it’s a question of when you’ll forget.”

Which makes perfect sense, it’s time well spent when we try to make our code streamlined and readable. You’ll end up with a cleaner, easier to maintain and more extensible code-base as a result. I mention this briefly here just to give some context, but I’m not planning to go into huge detail. It’s been covered many times and I’m assuming that we’re all on board with duplication is a code smell, and removing it is usually a good thing.

However, like most things, I believe this advice starts getting you into trouble if you take it to an extreme. Advice, just like code, does not live in a vacuum and context is king. When it comes to unit tests, sometimes the cost introduced by removing duplication can be greater than the benefits gained. What I plan to show you in this post are a few examples of where I would leave duplication in place and how this has, in my mind, led to cleaner and more readable code.

Peacock Programmers

[Edit – I’ve just discovered that an alternate name may already exist for this (and here I thought I was being original) – The Magpie Developer ]

I was trying to think up an appropriate term for a thought I had in my had (typically, a large chunk of time was spent coming up with a name) and settled on Peacock Programmers.

A Peacock programmer is someone who loves to use every tool in their arsenal and for various reasons attempts to decorate code with every possible feature that is available to them.

You can recognise the works of a peacock programmer by inelegant attempts at elegance. It’s wordy, flashy, looks complicated and almost impossible to understand without deep concentrated effort and untangling. Sure it’s fun to write and a blast to experiment and learn all the different possible ways of solving a problem, but that’s its use as an academic tool, when you’re seeing in on a production codebase, it’s just annoying.

Are Peacock Programmers a bad thing?

I was talking through the concept of Peacock programmers over lunch in the office and a colleague, Kai, raised an interesting point. The term sounds rather negative, and in truth my description in the previous section was slanted that way. However, Kai raises a good point. It is only through the efforts of these peacock programmers and excessive use of all these features that the rest of us learn where the limits of reasonable use are. It’s a common pattern with many technologies, like meta-programming for example. the first stage is to learn it. The next stage is to get excited by this new knowledge and attempt to use it everywhere. It’s then, by seeing it used excessively and understanding the consequences of that, that you learn to regulate its use and ensure it’s appropriate.

As Kai sees it, Peacock programmers are the early adopters that the rest of us ultimately learn moderation from, and their nature is an essential part of our learning.

A Case Study

The focus of this blog post is going to be a real unit test from a live codebase that I’ve worked on. I’ve copied the original test here below** as an example that the rest of this blog post is going to focus on. There were some more interesting examples to pick from, but they started to get a little unwieldy  for this blog post, so I’ve picked a simpler example. I’ll add some notes at the end on other issues I had found in the larger tests. I’ll also not be concerning myself with actual details of what is being tested, if it’s appropriate or well-formed, that’s not the focus here, but rather just the overall structure of the test class.

[** Disclaimer – the code and the style has been preserved exactly as was, I’ve just renamed classes/methods for anonymity]

This test was typical for the code base, and was seen by many as a well written one. In some ways it is, the tests are small, contained, and they test one thing each.

Yet I’ve always been frustrated when I see code like this. I’m a fan of simplicity and try to avoid, as much as I can, the use of superfluous language features. Maybe I’m just stupid (this is quite possible) but I look at at test like this and I really struggle to understand what’s going on. Also important to note is that I’ve picked one of the simpler samples for this post.

Nested describes – like a boss

we ran an analysis on a small set of our tests (the tests for one of our services) to figure out the worst culprits for nested describes/contexts. Our record was seven. That’s right, seven layers of nested description before you finally get to the test body. Good luck trying to figure out what’s actually being tested and finding the relevant bits of set up!

Nested describes/contexts can potentially be a way of helping structure your tests, but give it a moment’s though before you wrap that spec in a context and ask yourself “why am I doing this?”. Especially when you’re creating a context with just one test inside it, why not just make the test name more descriptive?  In isolation the idea of being able to use contexts to help structure and add meaning to your tests is a good one, and I’ve made good use of contexts. but use them sparingly, lift your head regularly from the depths of your spec and take a look at the whole test file and ask yourself how’s it looking?.  Once you start more than a couple of levels deep, perhaps it’s time to rethink and ask yourself why is your test so complicated?

e.g. instead of:

How about…

 Bonus:

Here’s an example I found from a codebase (again with the names obfuscated, but the only words I’ve replaced are farm and sheep, everything else is exactly as was) I’ve worked on with 6 layers (I couldn’t find the record-breaking seven-layer spec :( )

Now imagine the message you would see when this test fails…

‘Let’ it go

The other feature I think is worryingly overused is ‘let’. Now I understand that let is lazily evaluated and can improve performance, but we’re talking about unit tests here, which should be taking in the order of seconds to run a suite. What I’m more concerned about is the time wasted trying to decipher a test class that has let blocks scattered all over.

Again my biggest issue with them comes from misuse and misdirection. I have seen many simple unit test classes that have been made so much more verbose and complex through the addition of unnecessary let blocks. Their introduction led to an explosion in the scope of a test, no longer can I look at a self-contained “it” block and understand the test ,now I need to scroll all around, trying to piece together the nested contexts and follow the chain of let blocks to see which data is being set up, and then see which of these many lets my test is overriding…I’m getting anxious just thinking about it. (more thoughts on this, and alternative structure can be found in the When, Then, Given section below)

Subject

this is probably my least favourite feature, I can’t think of a single time in my own experience where I’ve thought that using “subject” was a good idea and am really confused as to why people are propagating it’s use. It just further reduces readability and adds pointless misdirection to your tests. When i first experienced the joys of RSpec after a stint working in Java, I loved the expressiveness and the flow of the test structure. This feels like a step backwards.

It gets even more confusing when your spec is testing something that is happening in the initialization of the class under test. (Note: bonus point for combining subject with described_class)

We came across a test like this by chance while working on a class, and my pair saw the trailing subject and assumed it was some mistake as lit looked like a pretty meaningless line accidentally added at the end of a long test. They deleted this line of code without giving it much though, and then a few minutes later when we ran the test, it naturally started failing.

My advice, ditch the pointless subject, new is not always better. Make your tests explicit, I’d much rather see an appropriately named variable that is instantiated within the test body it’s used as opposed to a cryptic “subject’ that was defined way up near the top of my test (or somewhere in the nest of contexts you had to traverse to reach the test)

When, Then, Given

I’m sure you’re starting to see a pattern here. Most of my annoyances are born from testing styles that increase the spread of focus that is required to understand what is happening. The idea of a unit test is that it tests a unit. I expect them to be concise and self-contained.
The way I try to achieve this is by following the “When, Then, Given” pattern. I’m assuming that you’re going to be familiar with the BDD given/when/then (or the synonymous arrange/act/assert) philosophy of test structure. When I write I unit test, I first start by mentally picturing these as three distinct sections of my test. At times, when trying to make this pattern explicit or share it with a new team I explicitly express the section through comments:

So far so good. However the mistake people make from here is to start working through the sections filling them in. I wouldn’t do that. Start with the When. This should 1 line, and the most obvious/easiest to write. When I look at a test written in this format I can immediately recognise the trigger for your scenario.
Next move on the Then. This is the next easiest section to fill in. What is the expected outcome?
And only then should you go back and complete the Given. In order to achieve this result, what data is required? And keep it all in the test! Again, this advice is flexible, and I can understand there are times when you’d want to pull things out, but I try to resist this urge as much as possible (in the rewritten version of the test at the end of this post, you’ll notice I did use one let block). Just keep it under check, else a few months later your test will have grown into a mess. keep the discipline.
This also brings us back to another reason why I hate let blocks. You can no longer read a spec and understand why the class under test is behaving the way it does. The data required to achieve this result is now spread all around. In addition to that, while you’re following all these let blocks to see what your test is doing, you’re chasing a load of red herrings as much of this data is totally irrelevant to this specific scenario.

Shared Examples = Shared pain

 I couldn’t find a small enough sample to include as a case study, but shared examples are just a nightmare! Imagine all the trouble of going back and forth across a long test file trying to make heads or tails of it, and then multiply that by 10. Just avoid.

Less duplication = more line of codes…right?

 This brings us back to our case study. I’ve attempted to rewrite a number of tests using this style, and the first time I tried, I truly did expect there to be more line of code. I was ok with that, I was willing to accept that as a trade-off for the increased readability and comprehensibility of the tests.

The original test was 162 lines of code.

The rewrite…101.

Technically speaking, there is more code, the lines are longer, but there are fewer of them, and they have more meaning to them. I’ve seen the same result in almost every test I’ve rewritten, and was surprised.

So here’s the finished result, this is the above test as I would have written it. It’s not perfect and perhaps it’s just me, but I find this style so much easier to comprehend and get my head around.

About the Author: akash

Leave a Reply

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