Relying on hash code implementation is BAD – part II

time to read 2 min | 325 words

To be truthful, I never thought that I would have a following for this post 4 years later, but I run into that today.

The following is a part of an integration test for NH Prof:

Assert.AreEqual(47, alerts[new StatementAlert(new NHProfDispatcher())
{
	Title = "SELECT N+1"
}]);

I am reviewing all our tests now, and I nearly choked on that one. I mean, who was stupid enough to write code like this?  I mean, yes, I can understand what it is doing, sort of, but only because I have a dawning sense of horror when looking at it.

I immediately decided that the miscreant that wrote that piece of code should be publically humiliated and  chewed on by a large dog.

SVN Blame is a wonderful thing, isn’t it?

image

Hm… there is a problem here.

Actually, there are a couple of problems here. One is that we have a pretty clear indication that we have a historic artifact here. Just look at the number of version that are shown in just this small blame window. This is good enough reason to start doing full fledged ancestory inspection. The test has started life as:

[TestFixture]
public class AggregatedAlerts:IntegrationTestBase
{
	[Test]
	public void Can_get_aggregated_alerts_from_model()
	{
		ExecuteScenarioInDifferentAppDomain<Scenarios.ExecutingTooManyQueries>();

		var alerts = observer.Model.Sessions[1].AggregatedAlerts;
		Assert.AreEqual(47, alerts["SELECT N+1"]);
		Assert.AreEqual(21, alerts["Too many database calls per session"]);
	}
}

Which I think is reasonable enough. Unfortunately, it looks like somewhere along the way, someone had taken the big hammer approach to this. The code now looks like this:

Assert.AreEqual(47, alerts.First(x => x.Key.Title == "SELECT N+1"));

Now this is readable.

Oh, for the nitpickers, using hash code evaluation as the basis of any sort of logic is wrong. That is the point of this post. It is a non obvious side affect that will byte* you in the ass.

* intentional misspelling