Don’t share constants between production and test code

Building upon my previous post and the trick of being specific in the values the code respects, one pattern that I’ve noticed which can easily lead in many false positive tests is sharing a constant value between production and test code.

If the test code reads the value from the production, any change that was done by mistake will not affect the test which will continue to pass!

21 yeas of age

Lets say that we have two services, one checks if a customer can enter a casino and the other if she can buy alcohol. For both cases the law states that the minimum legal age is 21 years old.

The code has a configuration file, a domain and two modules for each service:

	// Production code:

	// configuration
	object Config {
	const val MIN_LEGAL_AGE = 21
	}

	// domain
	class Person(val age: Int)

	// entrance module
	fun canEnterCasino(person: Person): Boolean {
	return person.age >= Config.MIN_LEGAL_AGE
	}

	// alcohol module
	fun canBuyAlcohol(person: Person): Boolean {
	return person.age >= Config.MIN_LEGAL_AGE
	}

	// Test code:

	// entrance module
	fun `a customer can enter the casino when she is older than 21 years of age`() {
	val twentyOneYearOld = Person(Config.MIN_LEGAL_AGE)

	val actual = canEnterCasino(twentyOneYearOld)

	assertTrue(actual)
	}

	// alcohol module
	fun `a customer can buy alcohol when she is older than 21 years of age`() {
	val twentyOneYearOld = Person(Config.MIN_LEGAL_AGE)

	val actual = canBuyAlcohol(twentyOneYearOld)

	assertTrue(actual)
	}

view raw dont_share_constants__sharing.kt hosted with ❤ by GitHub

As you can see the tests consume the minimum age directly from the production code but the test suite passes, life is good.

Then one day, the law changes and the minimum legal age for entering a casino drops to 20 years! Simple change, not much of a challenge for the old timers so the task is being given to the new teammate who does not know all modules yet and is also a junior software engineer.
She sees the test, changes the value in the name to 20, sees the config, changes the constant’s value to 20, runs the test suite, everything passes, life is good! Only that it isn’t because the casino’s software now allows selling alcohol to 20 year olds!

Keep them separate

If the test code did not use the production’s code

	// Production code:

	// configuration
	object Config {
	const val MIN_LEGAL_AGE = 20
	}

	// domain
	class Person(val age: Int)

	// entrance module
	fun canEnterCasino(person: Person): Boolean {
	return person.age >= Config.MIN_LEGAL_AGE
	}

	// alcohol module
	fun canBuyAlcohol(person: Person): Boolean {
	return person.age >= Config.MIN_LEGAL_AGE
	}

	// Test code:

	// entrance module
	fun `a customer can enter the casino when she is older than 20 years of age`() {
	val twentyOneYearOld = Person(20)

	val actual = canEnterCasino(twentyOneYearOld)

	assertTrue(actual) // passed
	}

	// alcohol module
	fun `a customer can buy alcohol when she is older than 21 years of age`() {
	val twentyOneYearOld = Person(21)

	val actual = canBuyAlcohol(twentyOneYearOld)

	assertTrue(actual) // failed
	}

view raw dont_share_constants__not_sharing.kt hosted with ❤ by GitHub

then, after changing the constant’s value, the test suite would fail alerting the software engineer that something has broken forcing her to figure it out and craft another solution.

Don’t force your objects to construct what they need

Let’s say we have an object that handles instances of Person. For example PeopleScreen:

	// Person.kt
	class Person(
	val name: String,
	val surname: String
	)

	// PeopleScreen.kt
	class PeopleScreen(
	private val people: List<Person>
	) {

	fun render() {
	people.forEachIndexed { index, person ->
	println("${index + 1}. ${person.name}, ${person.surname}")
	}
	}
	}

	// Usage:
	fun main() {
	val people = listOf(
	Person("Joe", "Dow"),
	Person("Jill", "Doe"),
	Person("Jack", "Black")
	)

	val screen = PeopleScreen(people)
	screen.render()
	}

view raw dont_force_construction__the_proper_way.kt hosted with ❤ by GitHub

PeopleScreen renders instances of Person so this should be the only format we provide to it. Let me explain.

Forced construction

There is a new flow that ends in opening PeopleScreen but all the information for the list of people are in a Map<String, String>. There is no reason to alter PeopleScreen in order to support this new format:

	// Person.kt
	class Person(
	val name: String,
	val surname: String
	)

	// PeopleScreen.kt
	class PeopleScreen(
	private val people: List<Person>
	) {

	constructor(people: Map<String,String>) : this(
	people.map { entry -> Person(entry.key, entry.value) }
	)

	fun render() {
	people.forEachIndexed { index, person ->
	println("${index + 1}. ${person.name}, ${person.surname}")
	}
	}
	}

	// Usage:
	fun main() {
	val people = mapOf(
	"Joe" to "Dow",
	"Jill" to "Doe",
	"Jack" to "Black"
	)

	val screen = PeopleScreen(people)
	screen.render()
	}

view raw dont_force_construction__the_forced_way.kt hosted with ❤ by GitHub

the new format is passed through an overloaded constructor but the same goes if we use a setter method

Why we shouldn’t do it

We could argue that by doing so we tie the object with each special format making the code hard to maintain and scale but the real reason is that we violate the SRP principle since PeopleScreen will have more than one reasons to change. One if something changes in the way we render and two if something changes in Person‘s construction.

What we should do

We should keep PeopleScreen only consuming Person and move all transformations to their own objects allowing a coordinator to transform and pass data around.

Good practices: First write the test then fix the bug

You get a report about a bug. You open the app, follow the steps to reproduce it and, as mentioned in the report, your app is misbehaving. What’s next?

You can either dig immediately in the code and fix the bug or you can re-reproduce the bug, only this time in a test. The second. Always go with the second option.

Here is why:

From now on you will have a regression test.
Meaning that if a change in the code breaks what you fixed you’ll get notified from the test suite and not your users
It keeps you focused / You know when you finished.
This is a benefit you get from TDD in general. When the test passes the bug is fixed and you can move to your next task. Also, since you have to make the test pass, anything else that popped up during your research for the bug can wait (I usually write it down to a notepad I keep next my keyboard).
You get a better understanding of the code.
By trying to write the test you get a better knowledge of how things are connected and communicate. Especially if you are new to a project this will boost your understanding significantly.
You discover more corner cases.
There are times that by writing this one test and seeing what inputs a class/function can have, you wonder how will the app behave under certain values. Finish the task at hand and then add a test for each case you want to explore. You might end up solving more bugs!