Your Tests Are Your Conscience

Have you ever inherited a project without any tests? Trying to get a foothold in the code can feel daunting, let alone feeling confident about making changes.

I recently took over a project that lacked any tests, and as I began to explore the code, I came to a realization. Despite the fact that the app had known issues and a general air of “bugginess,” the code wasn’t terrible. I’ve seen truly terrible code, and this wasn’t it. No, the realization I came to is that the code was about what you would expect, given that it had no tests.

You can understand your language inside and out. You can know the right ways to use MVC frameworks, ORMs, or whatever tools and libraries are involved. You can even deliberately incorporate useful practices like the single responsibility principle, dependency injection, and the uniform access principle. Even with all those tricks up your sleeve, if you don’t write tests, I firmly believe that your code will suffer.

Writing tests exposes the “pain points” in code. Without having to face down those “pain points” prior to discovery of bugs, it is all to easy to ignore code smells and press on. I know I am susceptible to the temptation. It really exemplifies the “one broken window” problem. Once your code is already a little sloppy, it is so much easier to just dump one more “tiny” hack in there than begin the painful process of refactoring. Besides, without tests how are you going to know that your refactoring didn’t break the code?

Here are just a few “paraphrases” of code problems I saw in the project I inherited, problems I attribute to the lack of tests.

Repetition

return array(
    'errors'          => array(),
    'entity'          => $entity,
    'form'            => $form->createView(),
    'alpha'           => $common['alpha'],
    'beta'            => $common['beta'],
    'gamma'           => $common['gamma'],
    'delta'           => $common['delta'],
    'epsilon'         => $common['epsilon'],
    'iota'            => $common['iota'],
    'stuff'           => $user->findStuff($db),
    'relatedEntities' => $related,
);

This particular block of code (plus or minus a few key/value pairs) occurs as the last statement in three or four action methods on a controller. I assume what happened is that this return statement was originally written for one action. Then, when another action required the same data at the end, rather than refactoring it out, it was quicker to copy and paste. However, if tests were being written during (or ideally before) the code was written, you’d have to do at least two copy and paste operations — the second one being for your test. Having to do something questionable twice in a row can help you repent of the evil you are about to commit and find a better way.

This code has a different — but closely related — issue. Half the keys in this array are supplied by another associative array: $common. When there were only one or two values coming from $common, maybe this was okay. But since the returned array uses all the keys in $common, it would be much more elegant to use array_merge. You would notice this in tests because every time you add a key to $common, you would have to add it to your tests. Whereas if you used array_merge, a handful of test cases would be sufficient for all the logical cases for merging an array into the result.

Overloaded Responsibility

In this app, the create function on the main controller is almost 200 lines long. The author was aware that this is not ideal, and even commented as much at the top of the function. However, he justified this smell by saying that the framework being used (Symfony 2.0) makes it difficult to keep controller methods “thin.” So, when it came time to add more business logic on record creation, the path of least resistance was to add more responsibility to the create function, rather than delegate elsewhere. However, if there were tests, there would be the massive pain of writing a unit test that has to mock and/or otherwise deal with the inputs and outputs for a dozen different functions and objects. My guess is that this would be more than enough to nudge the code writer in a better direction.

Spaghetti Code

Whenever you have a function that is too long, you are in immediate danger of spaghetti code.

if (count($errors) == 0) {
    if (!$entity->getSomethingRelatedToAlpha()) {
        $entity->setSomethingRelatedToAlpha('hello');
    }
    $entity->doBetaStuff();
    $coreEm->getRepository('Repo:Entity')->saveStuffFromPost($entity->getUser()->getSomethingRelatedToAlpha()->getAttachedData($coreEm), $post);
    if, (isset($post['gammaData'])) {
        $gammaData = $this->renderView('Stuff:Gamma:'.$entity->getTemplateName(), array(
            'post' => $post,
            'event' => $entity,
            'GammaOnly' => TRUE,
        ));
    } else {
        $gammaData = '';
    }
    $entity->setGammaData($gammaData);
    $betaIds = (isset($post['beta_record_ids'])) ? $post['beta_record_ids'] : array();
    $entity->doSomethingWithBetaIds($betaIds);
}

In addition to a Demeter violation that would never live through a mocking unit test, this code jumps between different areas of functionality repeatedly. Since there are no tests, if someone else has to refactor — or even understand — this code, they will be faced with the unenviable task of deciphering what the distinct areas of logic are in your code, and why some of them appear to be split apart across other logical areas. For example, why are the $betaIds used after the $gammaData section, and not immediately following doBetaStuff()? Is there a (completely obscure) dependency, or is this just bad organization? Tests — especially unit tests — make me consider the structure of my code in a deeper way that I might otherwise be tempted to sidestep.

As for me and my projects, I’ll be writing tests.

Without tests to be the Jiminy Cricket on your shoulder, it is far too easy to fall into the trap of making a little hack here, a little hack there. Even in a small app, those messes add up fast. It doesn’t take much for an application to feel “wobbly,” and then it is only a matter of time before painful refactoring or a re-write altogether.

It’s possible there are programmers out there so brilliant and so conscientious that they don’t need tests to help them. And if that’s you, then you’re a better programmer — and a better person — than I am. I swear by tests these days. If my framework resists testing, then if at all possible I will either ditch the framework or find a way to work around its limitations. It’s the right thing to do, and in the long run, I’m really just making life easier for myself.