To assert or not to assert, that is the question

16 Nov 2015

Re: To assert or not to Assert

This is a response to Misko Hevery's article “To Assert or Not to Assert”. This is one of the few things I disagree with Misko Hevery on. The article itself discusses whether or not the constructor of an object should check whether the arguments it has been supplied are valid. Hevery's reasoning is sound and it does highlight a real issue, however, in this case the issue is not where he has located it.

I happen to disagree with Hevery here and for good reason. His reasons for saying “We shouldn't code defensively” are that it makes unit tests harder to write. However, his example is severely flawed.

Hevery presents the following code as an example

class House {
  Door door;
  Window window;
  Roof roof;
  Kitchen kitchen;
  LivingRoom livingRoom;
  BedRoom bedRoom;

  House(Door door, Window window, Roof roof, Kitchen kitchen, LivingRoom livingRoom, BedRoom bedRoom) {
          this.door = Assert.notNull(door);
        this.window = Assert.notNull(window);
        this.roof = Assert.notNull(roof);
        this.kitchen = Assert.notNull(kitchen);
        this.livingRoom = Assert.notNull(livingRoom);
        this.bedRoom = Assert.notNull(bedRoom);
  }

  void secure() {
    door.lock();
    window.close();
  }
}

And makes the argument that with the assertions, the code is harder to test

Without the asserts it's possible to test the code like this:

testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window, null, null, null, null);

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

But with the asserts, more code is required:

testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window, new Roof(), new Kitchen(), new LivingRoom(), new BedRoom());

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

And while that's true, removing the asserts and allowing null values will cause much bigger issues than a minor readbility issue in the test-case

Hevery skirts around the underlying issue with his original code but seems to see only one side of this coin:

If the test fails here you are now sure where to look for the problem since so many objects are involved. It is not clear from the test that that many of the collaborators are not needed.

He goes on to say

Now let’s say that i want to test the secure() method. The secure method needs door and window. Therefore my ideal would look like this.

Which is where he provides the ideal test code:

testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window, null, null, null, null);

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

Exactly why this is flawed is hidden in plain sight in the single sentence: “The secure method needs door and window. Therefore my ideal would look like this.”.

If the test knows that the secure method needs only the door and window, then the test knows about the implementation of the secure method. This is bad because it means that if the implementation of the secure method changes, the test must be updated.

Consider a house with a back door in the Kitchen. In this case, the secure method needs to be changed to this:

void secure() {
    door.lock();
    window.close();
    kitchen.lockDoor();
  }

However, our test now breaks because the test “knew” that the secure method required only the door and the window, once the secure method is updated, the test needs to be updated as well. This is a classic case of a brittle test due to broken encapsulation and the only way around it is decoupling the test from the implementation details of the secure method. The test shouldn't determine how the house is secured, only that calling the secure method has secured the house.

The house should contain a method called isSecure() and the test should call it:

testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window,
             new Roof(), new Kitchen(), new LivingRoom(), new BedRoom());

  house.secure();

  assertTrue(house.isSecure());
}

By doing this, encapsulation has not been broken, the house object is in complete control of its state and what qualifies the house as secure or not is determined by the author of the class, not the author of the test. The test author doesn't know how the house is secured, it delegates that responsibility to the house object where it belongs.

What qualifies the house as being secure is now properly encapsulated and doesn't need to be expressed in the test. The author of the house object describes what makes the house secure, and the author of the test simply uses it. The test ensures that the object follows the correct API, the implementation is completely hidden from the test (and any code using the object).