5 Easy Ways to Write Hard-to-Test Code

Once I heard a talk where the presenter asked the audience how they would write hard-to-test code on purpose. People in the room were a bit shocked, as nobody would write bad code intentionally. As far as I remember, they couldn’t even come up with very good answers to this question.

Of course, the purpose of this question was not to educate people on how to code in a way that makes their colleagues’ lives harder. Knowing what makes code hard to test can help avoiding some serious problems. Here’s my list of answers to the above poll (of course it’s really subjective, almost everyone has a different set of smells that they hate most).

1. Use a Lot of Static Fields

Especially static collections that are shared between different classes. Like this one here:

public class Catalog {

    private static List<Person> people = new ArrayList<>();

    public void addPerson(Person person) {
        if (Calendar.getInstance().get(Calendar.YEAR) - person.getYearOfBirth() < 18) {
            throw new IllegalArgumentException("Only adults admitted.");
        }

        people.add(person);
    }

    public int getNrOfPeople() {
        return people.size();
    }
}

And now let’s see some tests:

public class CatalogTest {

    private final Catalog underTest = new Catalog();

    @Test(expected=IllegalArgumentException.class)
    public void addingAMinorShouldThrowException() {
        assertEquals(0, underTest.getNrOfPeople()); // precondition
        Person p = new Person(2015);

        underTest.addPerson(p);
    }

    @Test
    public void addingAnAdultShouldSucceed() {
        assertEquals(0, underTest.getNrOfPeople()); // precondition, or is it?
        Person p = new Person(1985);

        underTest.addPerson(p);
        assertEquals(1, underTest.getNrOfPeople());
    }
}

If you run these two tests, you will see that the one expecting an exception will fail. It is somewhat counter-intuitive, but JUnit is free to reorder test cases to force users not to depend on the order in which they are run. That is why in this case the second test runs first; it checks if the collection is empty, then successfully registers an adult. Then, since our collection inside Catalog is static, the second test case will see that the collection is not empty (we have a person in it, registered by the previous test case), thus the precondition fails.

Once we drop the static keyword of the list, both test cases complete successfully. Before each and every test run, JUnit reinitializes all the fields in a test case (not only the ones created inside the @Before method). As we now have an instance member instead of a static one, the people list is not tied to a class, but to an instance.  This means that before each test case a new Catalog object is created, within which a new list is created. Every time we start out with a new, fresh, empty collection of people.

Of course, in this case it is easy to detect and solve the problem, but imagine a large system where the list of people is written by several classes.

Static, modifiable collections -according to a former colleague of mine- are like trash cans; they may contain all sorts of garbage and should really be avoided.

2. Declare Most of the Classes Final

Making a class final prevents creating a mock of that class. This is what happens when an attempt is made to create a mock of a final class using Mockito:

org.mockito.exceptions.base.MockitoException: 
Cannot mock/spy class <CLASS_NAME>
Mockito cannot mock/spy following:
  - final classes
  - anonymous classes
  - primitive types
      at org.mockito.internal.runners.JUnit45AndHigherRunnerImpl...

Declaring a class final has serious consequences (for example: it’s impossible to extend or mock it), so it needs a good amount of reasoning before doing so.

All is not lost, however, when facing a final class. It only requires extra effort. Powermock is able to mock such classes, but I think this library treats the symptom only, not the problem. Another way could be creating a non-final wrapper class around the final one and mocking the wrapper class. This, of course, only works if you can get away with possible signature changes.

3. Always Construct Objects Inside Methods, but Mainly in the Constructor

I think this does not require much explanation. Creating objects within methods or constructors leaves us no way to introduce test doubles. Hard-coded dependencies completely disable mocking, thus keep us from writing true unit tests (fast tests for a class in isolation, without any kind of external dependency).

Dependencies should always be injected, mainly through the constructor. If that is not possible, for some reason, object creations can be pulled out into protected methods that can be overridden in a fake class that extends the original one.

4. Never Keep in Sync a Test/Method’s Body and Its Name

Many people think that long methods are one of testing’s biggest enemies. Although it is true in most cases, it is not a general thing. Just imagine a very clever and really long implementation of the square root function. It takes an integer and returns a floating point number. As we have a pretty good understanding of how square root works, it’s not necessary to care about the implementation details. We can treat the method as a black box and easily write tests for some well known values, (9,25, 36) and some less known values.

However, if the method is called log, for some reason, then we’ll try to write tests for an entirely different thing. That is a complete waste of time, as the test case(s) written for some method are completely useless.

The same is true for test methods. Once a test starts failing it is really important that its name describes what the test does. If the test method is something like throwsExceptionForNegativeNumbers and the test value is a positive one with no exception handling whatsoever, then it takes a lot of effort to first understand what we are even testing.

5. Never break stream operations into several instructions

Just because Java 8 streams go with the fluent interface, it does not mean filter, map, flatMap and the other operations have to be written one after the other, in a long chain (or better yet, nested one inside the other).

Let’s see an example. Given a list of soccer players, for each football club return the value of their strikers between 25 and 30 years:

public Map<String, Integer> someMethodName(List<Player> players) {
    return players.stream().filter(player -> {
            int now = Calendar.getInstance().get(Calendar.YEAR);
            return (now - player.getYearOfBirth() < 30) && (now - player.getYearOfBirth() > 25);
        }).filter(player -> player.getPosition() == Position.ST)
        .collect(Collectors.groupingBy(Player::getPlaysFor, Collectors.summingLong(Player::getValue)));
}

The problem here is that it’s really hard to see what kind of data should be passed into a method with such chains so it passes through all possible data routes. It is really hard to figure out what kind of player list should we use as input.

Even though tempting, long lists of operations make the code really unreadable. Usually it is a good idea to break them up into chunks and extract parts into variables or methods. Clean code rules apply.

Doing a few extractions, we can end up with something like this:

public Map<String, Integer> someMethodName(List<Player> players) {

    Stream<Player> playersInAgeRange = players.stream().filter(isPlayerInAgeRange());
    Stream<Player> strikers = playersInAgeRange.filter(isPlayerStriker());

    return strikers.collect(Collectors.groupingBy(Player::getPlaysFor, Collectors.summingLong(Player::getValue)));
}

private Predicate<? super Player> isPlayerStriker() {
    return player -> player.getPosition() == Position.ST;
}

private Predicate<? super Player> isPlayerInAgeRange() {
    return player -> {
        int now = Calendar.getInstance().get(Calendar.YEAR);
        return (now - player.getYearOfBirth() < 30) && (now - player.getYearOfBirth() > 25);
    };
}

Even though a little bit longer, visibility has increased an awful lot.