What Is That Smell?

Do not add an equals() method to a class to make test writing easier. I did just that a few months ago and have only just discovered the consequences.

I quite like JUnit assertThat() syntax. I wanted to write this test assertion:

assertThat(sut.collapse(rawResults),
    hasItems(expectedBloodPressure_1, expectedBloodPressure_2));

However, the test assertion failed to work when I first executed my test because I needed an equals() method on the Result class that treated Result like a value object. That is, I wanted to test for equality based on the values of all the Result fields. Without thinking through the consequences, I added this method:

public boolean equals(Object obj) {
  return EqualsBuilder.reflectionEquals(this, obj);
}

Nice. Using the Apache Commons EqualsBuilder.reflectionEquals() method was simple and clear.

End of story. New code tested with an expressive and easy to write unit test. Not quite.

In the same application, during an overnight process that synchronizes changes from a data warehouse, new results are extracted, transformed, and loaded into the application.

I was doing some performance analysis to try to speed up this process and I noticed that most of the time was spent talking to the data warehouse. No surprise there. I scanned down to locate the first method that was not in the JDBC driver or in the JDBC library and found the equals() method on Result. It was being called over 700,000 times. What had previously been a super fast address comparison was now a reflection based equals. Yikes.

Clearly, adding an equals() method to make test writing easier was a bad idea. Gerard Meszaros categorizes this smell as a special case of Test Logic in Production that he has named Equality Pollution.

So, what did I learn? One, no test logic in production code. Two, never do performance tuning without a tool that lets you see what is actually going on.

Leave a comment

Your comment