Add tests, discover things!

The benefits that you get from testing your code will never cease to amaze me.

I was working on a new feature and part of it required changing one of our oldest classes. The class was still in Java so I decided, before doing any other work, to convert it to Kotlin. There were no tests so my first step was to add them and make sure that the conversion wasn’t going to break anything.

The class is a simple configuration one that

• setups a couple of its fields upon construction and
• exposes its validity state

something like this

	public final class Configuration {
	private final String agent;
	private final String header;
	private final String version;
	public Configuration(final Storage storage) {
	this.agent = storage.get(KEY_AGENT);
	this.version = storage.get(KEY_VERSION);
	this.header = storage.get(KEY_HEADER) + "/" + VERSION_PREFIX + this.version;
	}
	public boolean isValid() {
	if (getAgent().length() == 0) return false;
	if (getVersion().length() == 0) return false;
	return getHeader().length() != 0;
	}
	public String getAgent() {
	return agent;
	}
	public String getHeader() {
	return header;
	}
	public String getVersion() {
	return version;
	}
	}

view raw discover_by_testing_initial.java hosted with ❤ by GitHub

but with many more fields.

Falsely valid

So I started writing tests to cover all cases and the one that failed immediately was

	@Test fun `the configuration is not valid when its header is empty`() {
	whenever(mockStorage.get(KEY_AGENT)).thenReturn("an agent")
	whenever(mockStorage.get(KEY_VERSION)).thenReturn("a version")
	whenever(mockStorage.get(KEY_HEADER)).thenReturn("")
	val actual = configuration().isValid
	assertFalse(actual)
	}

view raw discover_by_testing_failed_test.kt hosted with ❤ by GitHub

at first I thought I had an error in the test but looking closely at the production code I saw the problem. Actually the problems. Plural:

1. Even thought in this case its subtle, the constructor is violating the SRP. It initializes the fields and makes a decision on what will be exposed. This is a job for the getter.
2. Which brings us to the second problem. Getters are great way to expose information allowing us to make internal changes without breaking anything outside the class. They help us decouple ourselves from the classes that consume us.
   By using the getters instead of the fields, as in isValid, we couple the class with itself making it depend on what will be exposed and not the actual internal state of the class.

For the record the fix is

	public final class Configuration {
	private final String agent;
	private final String header;
	private final String version;
	public Configuration(final Storage storage) {
	this.agent = storage.get(KEY_AGENT);
	this.version = storage.get(KEY_VERSION);
	this.header = storage.get(KEY_HEADER);
	}
	public boolean isValid() {
	if (agent.length() == 0) return false;
	if (version.length() == 0) return false;
	return header.length() != 0;
	}
	public String getAgent() {
	return agent;
	}
	public String getHeader() {
	return header + "/" + VERSION_PREFIX + this.version;
	}
	public String getVersion() {
	return version;
	}
	}

view raw discover_by_testing_fixed.java hosted with ❤ by GitHub

and we managed to figure it out by writing a small, simple test.

Quick feedback

Having a quick feedback loop is a great way to keep yourself focused on the end result and to make less mistakes on the way. In our profession this is achieved with tests.

This bug was hiding for quite some time so the loop in this implementation was literally years!
Aim for shorter time periods! Seconds is the best and to do that is to write the test along side the production code.

I strongly encourage you to follow TDD but it doesn’t matter if its not your cup of tea. Write it after the production code, just do it immediately after.