- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Why do you need a found flag in the search results. Isn't it found if it's in the results?
Admin
My eyeses! It burns us!
Admin
Maybe he used to be an Oracle programmer? No SANE person would do that.
Admin
Not if the results contain all of the rows in the table w/o applying any criteria at all, as this genius has done.
Admin
Hey! That's my code! My beautiful code!
Just kidding.
Admin
Now that type of thing just makes me sick!!!
Admin
Hey, at least he's using WHERE's! Now granted theyre used in the most retarded way possible...BUT THEY'RE THERE!
Admin
That made my eyes hur. Question for the the people who do know this SQL procedure dialect: doesn't this also have a giant race-condition if two calls to sp_SearchProjects are made simultaniously?
Admin
I can find three nice things to say about this procedure:
- It is nicely formatted so that it is very easy to see what is being done.
- In some hardware configurations it may well run faster than selecting all rows and filtering them on the client side.
- It is encapsulated such that someone who actually understands how
to write a WHERE clause can replace this stored procedure with a decent
one without affecting any other code.
Of course, the whole procedure body could (and should) consist of a single SELECT statement, the format of which is so obvious that it's not even worth typing it in here. I'd suggest that the programmer here might be very competent in other types of coding, but shouldn't be left to generate SQL with some guidance.A programmer capable of this who claims any SQL expertise should be hung by their toenails and forced to watch episodes of "The Nanny" until they recant. Otherwise, this seems like a management WTF to have put this person on the task.
Admin
I think Alex's have run out of real WTFs and he is just making up crap. There is no way a human being can make code as heinous as this one. We have hit a new low, gentlemen.
/The goggles they do nothing
Admin
But, boss: I wanted to make sure the code followed the user requirements as closely to their original written form as possible...
Admin
This isn't so bad. At least it completes in O(N) time and memory usage.
A real WTF would have to use at least two tables to be O(N*N) in either time or memory. Possibly in both if you really bugger it up.
Admin
Can you really declare a table with @Table syntax?
Admin
The Torvaldian programmer has heroically avoided a race condition by using all local variables.
Admin
Wow, this is the first time I literally had the words "WTF?" pop out of my mouth while reading code. Perhaps because I do T-SQL by trade. Good thing nobody overheard me; I deem this Not Safe For Work!
This seems to be Microsoft SQL Server's T-SQL, and all of the rows are selected into a "table variable" scoped for the stored procedure. There probably won't be any lock contention or anything since it doesn't go back to the original table. If it's a big table, there might be a problem with all the reads it has to do, especially if the table sees a lot of write activity as well... But if it's a big table, and the stored proecdure is called a lot, memory will probably be a big problem too.
Admin
If this is MSSQL then definitely:
DECLARE @TT TABLE (
Account varchar(40),
BOOKDATE INT
)
Admin
I don't think it's ANSI compliant, but SQL Server lets you use in-memory "table variables" local to a batch or stored procedure.
Admin
If this is MSSQL then definitely:
DECLARE @TT TABLE (
Account varchar(40),
BOOKDATE INT
)
Admin
Yes. You get an in-memory local-to-the-procedure table rather than a temporary table which actually resides the database.
No. Because of the above, every call to the SP works with its own copy of the data.
I suspect this is what happened--the code is just too structured to be a newbie programmer mistake. It screams "I'm new to SQL" not "I have no clue how to code". We all have our moments--I've just recently become more in tune with how to use creative joins to get the results I want rather than resorting to a cursor/loop structure. There's some less-than-efficient stored procedures in our SQL server right now thanks to me, but I've learned from my mistakes and I'm correcting my code as I come back across it.
Admin
Sure! This is MS SQL Server dialect, and all that declaration does is create a variable of type Table. It is not actually creating a table in the database. The closest description is that it is an in-memory table (that is not completely accurate, but close enough).
Not that it matters, of course, since this is one of the most painful ways I have seen to filter data![:S]
Admin
Anonymous pretty much said it all, but in addition I'd like to offer a humble plea to all MS SQL Server developers to stop giving their stored procedure names an sp_ prefix. When you call a procedure that begins with "sp_", SQL Server assumes it's a system stored procedure, and looks for it in the master database first. If it doesn't find it there, then it looks in the current database. So by calling it sp_blahblah, you're adding lots of useless overhead.
Of course, this is yet another reason why Hungarian notation is getting slowly killed off. Developers think, "Hey, this is a stored procedure, so I better name it 'sp' for 'stored procedure'! I know that the object is in a Stored Procedures folder in Query Analyzer/Enterprise Manager, and the script begins with 'create procedure', but I better tack on this useless naming convention, just in case!"
Geez. Keep it simple, people: "create procedure dbo.ProjectSearch ..."
Admin
Wizard thank you so much for the comment to MS SQL devs.
Hungarian notation drives me nuts.....
Admin
OK, so in a stored procedure,
DECLARE @Table TABLE (...)
is effectively the same as
CREATE TABLE #Table (...)
where the # means "temporary to connection"
Learn something new every day...
As regards sp_:
sp_ is only useful if the following conditions both hold:
I've only been tempted to create my own sp_ procedure twice. The first time was before I knew any better. The second time I created an sp_send_cdosys_mail stored procedure for sending HTML-formatted email. It's probably the single-most-called procedure on my database server.
Admin
Why would you use the database to send e-mails?
I'm curious, not trying to flame :)
Admin
When your DBA revokes everyone's rights to creating objects in the database, this is the reason why. Don't complain when it happens, just walk away and say, "Yes, I see why things must be this way..."
</FONT>Admin
A million reasons.
A select few:
Online work-ticket system - when a ticket is added or modified, an email goes to all the relevant people containing details of the addition/modification and a link to the full history of the ticket.
Online advertising management system - when an ad reaches it's end-of-run, an email goes out to the interested parties including a summary of the ad's run and a link to performance stats
Shipping of physical products - when somebody buys something that requires physical fulfillment, an email goes to the department whose job it is to stick the thing in a box and send it out
etc.
Admin
Let me offer a couple of possibilites as to what this coder might have been thinking. Please note that I'm not justifying this code -- just trying to understand what the person was thinking (if anything).
2) Perhaps the person believes that having "OR" conditions in a WHERE clause causes SQL Server to do things too slowly. I've been told this is sometimes true, although I haven't seen it. Of course, I doubt it could ever be slower than this solution...
Just some thoughts.
Admin
Not *quite*. They behave a little differently.
http://support.microsoft.com/default.aspx?scid=kb;en-us;305977
Admin
No one. NO ONE, could be that stupid. I call shenanigans!
Admin
Holy gaaaawd!
dZ.
Admin
I should have phrased my question differently: Why would you have the database send the e-mails rather than in the middle-tier of your application? It just seems that it's a business role that you wouldn't want the database to be performing.
Admin
I can only think of one reason why you would want all of this in the database: Lack of a middle-tier. Why everyone wants to implement programs wearing the shackles of T-SQL when rich languages like C# or even VB6 (by comparison) exist is beyond me.
Admin
That could be the flaw in my thinking - I'm just assuming that there is a middle-tier. Of course, some smaller projects may not require one, but the examples given seem to indicate a scope that would need at least one middle-tier.
Admin
I guess you're against xp_sendmail too, then?
Admin
Mmmmmm.....WTF....*drool*
Thanks Alex, this was one of the best yet![:|]
Admin
This violates the principle of "right tool for the right job."
A database is a good place for keeping data. It is not a good place for anything else, including sending e-mail, generating HTML pages, number crunching, filtering network traffic, scanning for viruses, processing 3D images, or message queuing.
Admin
Need is such a strong word =). I've seen some pretty large systems implemented with the absolute thinnest of middle tiers. Even a bank...
Admin
Some people run batch jobs on their data. Sql server has a built-in scheduler and the easiest thing you can have it run is a sproc. Sometimes you want your batch script to send an email. People work with the tools they know, and there are a lot of people who know SQL. Especially the type of people who would be asked to write such a script.
I don't believe in putting much logic in the Sql server any more, but it's a religious bias.
I have heard again and again the argument that the database will likely outlive the API paradigm that is used to interface with it. I believe it's true. For me that's not a good enough argument not to use C# for the middle-tier, but I have a hard enough time convincing LOTS of people that a middle tier is a good thing. Many people (DBAs, older analysts, managers) know and trust the database, but don't get OO.
Many projects are small projects and it's harder to justify the benefits of OO.
I don't think this person is stupid. They know how to construct a valid stored procedure, and a moderately complicated one at that. They do know how to use a where clause and construct temporary tables. I think they had another reason for making this code.
Maybe they have some form of Autism or OCD that drives them to write code like this. Maybe they were incented to take a long time making the code. Maybe they were angling for a more powerful database server, so they made an awful pig of a sproc.
You can't deny that the code is well formatted and easy to read. It is as if they wanted to control every step of the process despite the obvious impact on the performance.
It's hard to explain why the developer not only avoided selecting by where clause, but also avoided deleting by where clause and used the bit flag to determine deletes. It's pretty paranoid behavior.
I wish the original poster could give some clue as to why this convoluted approach was used.
Admin
Others offered "lack of a middle tier in simpler systems."
There is seperation of duties. If you have an expert DBA who might want to change table structures, by encapsulating logic in a stored procedure he does not have to have the expert middle tier programmers change all of their code. The expert middle tier programmers don't need to make decisions about how to write queries (and writing them as efficiently as possible can require some expertise).
There is avoiding database roundtrips. Tree-structured data can require recursive database calls that might be better handled on the database side.
Specific to the email question: if you have multiple front ends, or different ways that a ticket/order/etc. can be indirectly changed, it might make sense to have notification emails fire based on a trigger on a base table. This is perhaps indicitve of poor planning of the system, but anyone who's worked in the "real world" for a while will tell you that systems grow and mutate unexpectedly, or you inherit other people's "WTF's?" and re-architecting everything is not cost effective.
Don't get me wrong, T-SQL is not a rich language by a long shot. But if you have to pound in nails sometimes, you'll appreciate having a $10 hammer in addition to a $200 ratchet set.
Admin
FWIW, I've never yet found it necessary to use a trigger (triggers scare me.) I have a wide variety of middle-tier languages in use
The stored procedure creates a CDO.Message object using sp_OACreate, sets various properties, and finally calls the Send method.
Admin
It looks like what the author was trying to accomplish was to allow one procedure to handle zero or more selection criteria (the IF LEN(@sProjectName) > 0 allows the caller to include or leave out the project name criterion, for example). Of course, this could still be accomplished with a single SELECT (using ISNULL in the WHERE clause for each criterion...)
Admin
There's a bit of debate on this thread on whether email, HTML, business logic, etc should exist on the database tier. My question to those that promote the "absence" of a middle tier is: what DON'T you put into the database, other than the presentation layer? My last job put all kinds of business logic in stored procs: sending & creating HTML emails using T-SQL cursors, randomly generating passwords, etc. It was a maintenance nightmare...
Also, keep in mind that in some cases, its the database gets replaced, rather than the client programs written in C#, VB, etc.
I think this is why we really need business object servers, or OOP databases or something. T-SQL just sucks at these kind of things. The next version of sql server is going to have the .NET runtime in it, but i guess thats a two edged sword.
Admin
You only put in the database what "belongs" in the database. This varies from app to app.
A good rule of thumb - for me - is to treat stored procedures as if they were method calls in an OO solution. They should be agnostic of how they are called, and do what their names imply.
Admin
I think the problem stems from the "6 optional parameters for a search" requirement. I've seen various standard solutions proposed before:
1. isnull in the where clause: where isnull(@city,city) = city AND isnull(@customer, customer) = customer AND...
2. OR in the where clause: where (@city is null OR @city = city) AND (@customer is null or @customer = customer) AND...
3.using if statments to break it out into 2^n cases where n = number of optional parameters.
#1 has poor performance but is highly maintainable and readable
#2 has better performance and remains highly maintainable and readable
#3 has the best performance but is the most difficult to read and maintain
My favorite is #2 but people tell me to use #3 because of the performance boost (I ignore them). Notice that every solution has drawbacks and so any standard solution you pick could easily be forbidden because of the drawbacks. So after reading a whole bunch of "don't do this ever" advice, you could easily fall into the trap of avoiding the known pitfalls and ending up with a monstrosity which is worse than any standard solution.
I've noticed that people have not volunteered some "better solutions" to the "6 optional parameters" requirement. Perhaps because every solution stinks so far. Though this has by far got to be the worst I've seen. Thanks for another great WTF Alex.
Admin
Another option is to (shudder) DYNAMICALLY BUILD a sql string WITHIN the stored procedure, and then run it...
DECLARE @sql nvarchar(4000)
SELECT
@sql = '...' +
'WHERE 1 = 1 ' +
CASE WHEN @ProductID IS NULL THEN ''
WHEN @ProductID = '' THEN ''
ELSE 'AND ProductID = ''' + Replace(@ProductID, '''', '''''') + ''' '
END +
CASE WHEN @ProductName IS NULL THEN ''
WHEN @ProductName = '' THEN ''
ELSE 'AND ProductName = ''' + Replace(@ProductName, '''', '''''') + ''' '
END
ew... slow and hard to maintain.
Admin
I don't know if anyone is "promoting" the absence of a middle tier, just pointing out that there is a cost to implementing a more complex system. For a smaller system, the efficiencies of a three-tier system might not make up for the cost of an expert programmer's hours. Some of us are implementing systems alone, or as part of a very small team. That doesn't mean that the database is the best place to generate a random password; but when choosing between two tiers, it might be the best place to send an email.
Sometimes the database gets replaced, but in practice I believe that happens less often then replacing a client program. Often I think you'll see several database clients. One makes the best predictions one can based on what one knows.
Implementing anything object-oriented does indeed suck in T-SQL, or really, in any relational database. I've seen more than one person try to use a system (with tables like "ObjectTypes", "ObjectAttributes", and "Objects") to try to fake it. There are object oriented databases (hit Google) and I'm curious whether any will gain significant market share.
.NET in Yukon SQL Server 2005 will open the door to all kinds of great WTF's, I'm sure. Things that should be done in T-SQL gratuitously being written in .NET, code that should NOT be on the database layer being done there... but the inclusion of an XML column type will be even worse, I bet. How many tables-inside-a-column will we see?
Admin
As an aside: Now I'm no DB expert, but I tend to default to #2 until the performance kills me. I don't know what it is about such queries, but MS SQL Server seems unable to parse out the expression (null is null OR null = city). When the performance becomes unbearable I simply switch over to dynamic SQL. It sucks, it's hard to maintain, but I don't know any better way.
There's got to be something I'm missing. Perhaps it's the fact that @city can change from row to row and the query planner can't automatically recognize that certain variables are immutable for the duration of the query. Is there a way to declare them immutable so that #2 is as performant as dynamic SQL?
In my experience, the dynamic SQL method by far the most performant for non-trivial search methods. What would you do?
Admin
NULLIF(@ProductID, ProductID) IS NULL AND
NULLIF(@ProductName, ProductName) IS NULL AND...
3. I have never attempted
For small tables I do 1. or 2.
For large tables I do 3.
Admin
That is to say - for large tables I do dynamic SQL
Admin
Damn f*ing priceless!