Wednesday, January 16, 2013

On Conversion, Data Types, and Flawed Code

I came across the following expression recently:

DisplaySequence = Convert(int, IsNull(A.DisplaySequence, 10000))
I was immediately struck by an inconsistency: the IsNull function is being passed an int as its second parameter. That doesn't make sense. If the Convert is needed in the first place, then the DisplaySequence column is not int already, so either the 10000 is going to be converted to some non-integer data type, or the DisplaySequence column will be converted to an int. Neither of those make any sense. If Convert is needed at all, it should come before the IsNull, so that it is the correct data type to not possibly perform strange conversions to one or another of the IsNull operands' disparate data types.

Also, if this is working in production code and hasn't been causing errors or improper ordering, then all the values in the DisplaySequence column, when not NULL, are already integers or an equivalent data type (as far as ordering is concerned)! And if any could be non-integers, then some way to extract the numbers from them or convert them to NULL would be needed. So all in all, this expression inherently makes no sense.

But let's address the question--could this column contain non-integers? A quick sp_help TableName reveals that the column is... drumroll please... int. So the Convert is definitely unnecessary, because the column is already int!

Why does it matter to make your expressions make sense? It was working, wasn't it, without any errors in production code? Who cares?

Well, it has been said that any programmer can write code that a computer can understand. But it takes a good programmer to write code that a human can understand. The very fact that I had to go and check the data type of the DisplaySequence column is proof that there is a cost to this kind of thing. There is no way for anyone to know that an inherently flawed expression is immune to error, and this prompts research on the spot to see if so. Inherently flawed code expressions should be corrected, immediately. My work to correct this today is paying down the technical debt that was taken out when it was first written--and now the next visitor to this code won't have to waste time.

Here is the correct expression:

DisplaySequence = IsNull(A.DisplaySequence, 10000)

Now there is no inherent contradiction and we can implicitly trust that DisplaySequence is of type int. Even if wrong on this point, we've removed the eyesore that would trip up the next visitor to the code.

No comments: