CodeSOD: Going the Distwince

Stored procedures are a bit of a WTF breeding ground. Your average stored procedure language is more limited than your average general purpose language, the syntax is usually clunkier, and everything is so data-focused that implementing logic beyond simplistic processes can be more challenging than one would expect.

Of course, this gets harder if you don't bother to learn how to do string concatenation. Darrin found an example of that while debugging a store procedure:

if @_DistType = 1 beginset @_Query = dbo.GetQuery('DistType1');execute sp_executeSQL @_Query endif @_DistType = 2 begin set @_Query = dbo.GetQuery('DistType2'); execute sp_executeSQL @_Query endif @_DistType = 3 begin set @_Query = dbo.GetQuery('DistType3'); execute sp_executeSQL @_Query end-- snipif @_DistType = 74 begin set @_Query = dbo.GetQuery('DistType74'); execute sp_executeSQL @_Query endif @_DistType = 75 begin set @_Query = dbo.GetQuery('DistType75'); execute sp_executeSQL @_Queryend

Note how each branch of the ifs is checking the @_DistType variable, and then running a stored procedure with a parameter in the pattern 'DistTypeNN'. That means there are 375 lines of code here which could be condensed down quite a bit. Now, I'm going to assume that this is SQL Server (what with the DBO prefix), so"

SET @_Query = dbo.GetQuery('DistType' + CAST(@_DistType AS CHAR))EXECUTE sp_executeSQL @_Query;

Now, my syntax may not be perfect, here. There are some good odds that I'll have to fight a bit with type casting, and there's probably gonna be some issue with passing a variable directly into a function, because y'know, there almost always is some gotcha on these kinds of queries. I'd have to poke at this a bit to make 100% certain that it works.

But what I'm not about to do is copy and paste the same block of code with minor variations.

And as angry as that makes me, you know what? That's not even TRWTF. dbo.GetQuery is clearly looking up a SQL statement from a table in the database, and then we're executing it. It's not quite as bad as trying to build a query on the fly through concatenation, but it's impossible to look at this block and have any idea what it's actually doing. And since GetQuery is probably looking up in a table, what it does could change without any code ever changing. This usually is taken to mean the code is "flexible" but the reality is that it's "unmaintainable".

[Advertisement] Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.