CodeSOD: It’s Not Wrong to Say We’re Equal

Aaron was debugging some C# code, and while this wasn’t the source of the bug, it annoyed him enough to send it to us.

<span class="hljs-function"><span class="hljs-keyword">protected</span> <span class="hljs-keyword">override</span> <span class="hljs-built_in">int</span> <span class="hljs-title">DoCompare</span>(<span class="hljs-params">Item item1, Item item2</span>)</span>
{
	<span class="hljs-keyword">try</span>
	{
		DateTime thisDate = ((DateField)item1.Fields[<span class="hljs-string">"Create Date"</span>]).DateTime;
		DateTime thatDate = ((DateField)item2.Fields[<span class="hljs-string">"Create Date"</span>]).DateTime;

		<span class="hljs-keyword">return</span> thatDate.CompareTo(thisDate);
	}
	<span class="hljs-keyword">catch</span> (Exception)
	{
		<span class="hljs-keyword">return</span> <span class="hljs-number">0</span>; <span class="hljs-comment">// Sorry, ran out of budget!</span>
	}
}

Not to be the pedantic code reviewer, but the name of this function is terrible. Also, DoCompare clearly should be static, but this is just pedantry.

Now, there’s a lot of implied WTFs hidden in the Item class. They’re tracking fields in a dictionary, or maybe a ResultSet, but I don’t think it’s a ResultSet because they’re converting it to a DateField object, which I believe to be a custom type. I don’t know what all is in that class, but the whole thing looks like a mess and I suspect that there are huge WTFs under that.

But we’re not here to look at implied WTFs. We’re here to talk about that exception handler.

It’s one of those “swallow every error” exception handlers, which is always a “good” start, and it’s the extra helpful kind, which returns a value that is likely incorrect and provides no indication that anything failed.

Now, I suspect it’s impossible for anything to have failed- as stated, this seems to be some custom objects and I don’t think anything is actively talking to a database in this function (but I don’t know that!) so the exception handler likely never triggers.

But hoo boy, does the comment tell us a lot about the codebase. “Sorry, ran out of budget!”. Bugs are inevitable, but this is arguably the worst way to end up with a bug in your code: because you simply ran out of money and decided to leave it broken. And ironically, I suspect the code would be less broken if you just let the exception propagate up- if nothing else, you’d know that something failed, instead of incorrectly thinking two dates were the same.

[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Source: Read More