The Daily WTF: Curious Perversions in Information Technology

2006-01-24 Reply Admin

Oh my god... This is probably the worst code I've seen posted on this site...

2006-01-24 Reply Admin

first post!

Ytram · 2006-01-24 Reply Admin

My guess is that it was some kind of crappy manual identity field management. Kinda like handling an identity by using MAX(IdField) + 1.

2006-01-24 Reply Admin

I wonder what would happen if they removed a user?

2006-01-24 Reply Admin

For me, project horror stories are pretty interesting, although, I agree that it probably isn't appropriate for this particular site. It can be a real drama to see how things wound up after such optimistic starts, what happened, what happened to the people on the project and the efforts to save it. Sort of "The Dark Night of the Soul of a New Machine".

nobody · 2006-01-24 Reply Admin

Punch line..

This is a punch in the face of any professional developer.

Rick · 2006-01-24 Reply Admin

Its obvious that the 'designer' wanted to save the unnecessary space that would be used up by a UserId column in the table. You can always determine the UserId with the code below.

<font>select * from USER_TABLE where UserName < "JOE BLOW"
SET</font> @intUserId = <font>@@ROWCOUNT

I consider this to be a very 'normal' way of doing things.

</font>

2006-01-24 Reply Admin

Oh... that's not good.

GoatCheez · 2006-01-24 Reply Admin

Alex Papadimoulis:
When you're working on a system that's stagnated in the development phase for a few years and has chewed through more developers and teams than you even realized existed, it can be a daunting task just to explain why the system is as ridiculously bad as it is. As much as I'd love to feature such systems here, I suspect that most would prefer not to read thirty-seven pages just to get to the punch line. No less, Rick Harris was able find a *single line* of code that is a fair representation of such a system he was working on ...
SET @intUserId = @@ROWCOUNT
And no, this was not taken out of context. And yes, it really was that bad. I'll leave it as an exersize for the reader to try to immagine what possibly could be surrounding this code.

I'm assuming that userid needs to be unique, and that this code is somewhere in the useradd portion... And for each user that is added, there is a row added to some table, before userid gets set for that user.... really don't do alot of db programming tho... But, to me, that's the ONLY logical explanation for this garbage... Well, at least that I can think of right now...

2006-01-24 Reply Admin

Anonymous:
I wonder what would happen if they removed a user?

I'm sure they found out the hard way, several times.

2006-01-24 Reply Admin

I would guess the code looks like:

set @strName = 'Wiley Hacker'
insert into users (name, ...) values (@strName, ...)
select * from users where 1 = 1
set @intUserId = @@rowcount
update users set UserId = @intUserId where name = @strName

Sure he could use identity fields, if he knew they existed. But this adds the excitement of concurrency issues and the performance hit of retrieving the whole table to get the row count.

RogerC · 2006-01-24 Reply Admin

Anonymous:
Anonymous:
I wonder what would happen if they removed a user?

I'm sure they found out the hard way, several times.

And since that onerous line of code is still in there, I suspect they worked their way out of the ensuing mess by manually whacking at records until everything was back as it should be, and then patting themselves on the back for another job well done.

I've seen it happen. More than once.

OneFactor · 2006-01-24 Reply Admin

Ytram:
My guess is that it was some kind of crappy manual identity field management. Kinda like handling an identity by using MAX(IdField) + 1.

That was my guess too. I must confess that I've done crap like max(id)+1 or select count(*)+1 from blah when needing to do ad hoc inserts to a table that has a primary key field int rather than identity. It doesn't seem too bad because it was done on the dev database where concurrency is not expected and when we migrate to production, we shut down the applications first the the DBA runs the diff script generated by Adept SQL.

I'm guessing the context is some dinky admin application to allow a single admin to insert a new user and assign an id. This doesnt sound like it is necessarily that bad. What do others think, is it really that bad?

yes, no, unknown, not specified, null, filenotfound, aSuffusionOfYellow

Gene Wirchenko · 2006-01-24 Reply Admin

Alex Papadimoulis:
No less, Rick Harris was able find a *single line* of code that is a fair representation of such a system he was working on ...
SET @intUserId = @@ROWCOUNT

The line of code is quite telling. My compliments go to Mr. Harris on his obviously excellent summarising skill.

Sincerely,

Gene Wirchenko

marvin_rabbit · 2006-01-24 Reply Admin

Everytime I read through these things, I just feel DIRTY! I have to go and answer a few questions at Experts Exchange to make me feel better.

ammoQ · 2006-01-24 Reply Admin

OMG... i hope this is a small, rarely changed table. (No, I won't try to explain or even excuse that. I've already had my dose of trolling today ;-)

ferrengi · 2006-01-24 Reply Admin

This is really not enough to work with.
At best it could be a silly but relatively harmless line of code. On the other hand, it could cause some major problems. I'm guessing major problems since Rick Harris took the time to submit this.

In the future, please provide us with a little bit more detail.
Or at least post a follow up after a few hours with more information.
This is one of the highlights of my day and I want to be able to grasp just how the much the code sucks.

Thanks.

OneFactor · 2006-01-24 Reply Admin

Anonymous:

I would guess the code looks like:

set @strName = 'Wiley Hacker'
insert into users (name, ...) values (@strName, ...)
select * from users where 1 = 1
set @intUserId = @@rowcount
update users set UserId = @intUserId where name = @strName

Sure he could use identity fields, if he knew they existed. But this adds the excitement of concurrency issues and the performance hit of retrieving the whole table to get the row count.

Maybe he was trying to attach child records (like user preferences) to the user record and was having trouble figuring out how to reliably determine what the autonum generated for the inserted record. I seem to recall it wasn't always obvious how to get that number (in fact, I do not know how to do it and would appreciate very much if someone could enlighten me). On my current project we've been using Guids and so it can be generated before the insert and passed to the insert parent statement and the insert children statements and I do not need to worry about the "what is the id of the row I inserted?" problem. Last project was Oracle and it used sequences and I was also sheltered from the problem. On my first project I knew the more senior developers were struggling with it and I do not know what they did about the problem.

Satanicpuppy · 2006-01-24 Reply Admin

OneFactor:
Ytram:
My guess is that it was some kind of crappy manual identity field management. Kinda like handling an identity by using MAX(IdField) + 1.

That was my guess too. I must confess that I've done crap like max(id)+1 or select count(*)+1 from blah when needing to do ad hoc inserts to a table that has a primary key field int rather than identity. It doesn't seem too bad because it was done on the dev database where concurrency is not expected and when we migrate to production, we shut down the applications first the the DBA runs the diff script generated by Adept SQL.

I'm guessing the context is some dinky admin application to allow a single admin to insert a new user and assign an id. This doesnt sound like it is necessarily that bad. What do others think, is it really that bad?

yes, no, unknown, not specified, null, filenotfound, aSuffusionOfYellow

If you're inserting a user into a database, it is always always always better to let the database figure out what number to use, through whatever auto-incrementing key you've setup to take care of the user table. That is the only way you can make sure it's unique.

I had a situation once, where I'd run a bunch of bug testing and added a bunch of users, then removed them, so the counter was set at a higher number than the last existing row. So some bright boy comes along and decides that he's too smart to add stuff to the table using the methods that were set up for that...he's going to write a query to dump all his new users directly into the tables. So he figures out the last entry, and inserts all his users, setting their FOREIGN KEYS in related tables to what he imagines their ID numbers will be. Then, when regular users get added, we get duplicate foreign keys. Total snafu.

Stuff like that is very easy to do with a database. Use built-in methods wherever possible.

2006-01-24 Reply Admin

Anonymous:
Anonymous:
I wonder what would happen if they removed a user?

I'm sure they found out the hard way, several times.

It is a user stack, not a user table. This code is safe as long as you remove users only at the end of the table. If you want to remove a user that isn't the last user on the table, you just remove the
users after him in the reverse order, remove the user in question, and re-add all other users to the table. You may do this easily using a recursive function. :D

Hugo · 2006-01-24 Reply Admin

OneFactor:
how to reliably determine what the autonum generated for the inserted record. I seem to recall it wasn't always obvious how to get that number (in fact, I do not know how to do it and would appreciate very much if someone could enlighten me).

Hi OneFactor,

Since an autonumber (or IDENTITY, as SQL Server calls it) should never be the only key for a table, the logical way is to read back the record using the business key to get the generated identity value.

Alternatives are SCOPE_IDENTITY(), @@IDENTITY, and IDENT_CURRENT('table_name'). They are described in Books Online.

Best, Hugo -trying hard to forget today's WTF code...

2006-01-24 Reply Admin

Depends on the database, but in SQLServer for example, you can use "SELECT @@IDENTITY" to get the last autogenerated key.

Sean · 2006-01-24 Reply Admin

Maybe this was a contract for some touchy-feely government agency. They didn't want any users to feel alienated by having a different
ID than the other users. Did Rick Harris even look at the project requirements before submitting this code???

2006-01-24 Reply Admin

Anonymous:
Depends on the database, but in SQLServer for example, you can use "SELECT @@IDENTITY" to get the last autogenerated key.

That's not scope sensitive, SCOPE_IDENTITY is

osp70 · 2006-01-24 Reply Admin

maybe for security reasons they can NEVER delete rows from the employee table.

maybe I'm going to rub some salt in my eyes and try to forget I ever saw this.

OneFactor · 2006-01-24 Reply Admin

Hugo:

OneFactor:
how to reliably determine what the autonum generated for the inserted record. I seem to recall it wasn't always obvious how to get that number (in fact, I do not know how to do it and would appreciate very much if someone could enlighten me).

Hi OneFactor,

Since an autonumber (or IDENTITY, as SQL Server calls it) should never be the only key for a table, the logical way is to read back the record using the business key to get the generated identity value.

Alternatives are SCOPE_IDENTITY(), @@IDENTITY, and IDENT_CURRENT('table_name'). They are described in Books Online.

Best, Hugo -trying hard to forget today's WTF code...

Yeah, I looked at that after making my post. It wasn't clear to me that any of the three options was robust. For Scope_Identity() it had a warning about triggers in the insert which would mean that it would bust if someone started created insert triggers. @@Identity and ident_current('table_name) seemed like it was rather "global" and vulnerable to concurrency issues if another transaction did an insert during the "lag time" between the original insert and the call to the identify variable or function. Perhaps I read the documentation wrong but my fears were not soothed by what I read. Finding three options each with differing warning labels made me nervous about the possibility of inadvertent error. I feel a bit like Brave Sir Robin running from the three-headed giant.

I noticed a lot of suggestions to generate the Guids before doing the insert (which is what we currently do on this project) and that has worked out very well for us. Asking for a Business Key feels like wishing Santa and his Reindeer would give me a lift to work every day so I can avoid traffic.

Guid, Guids, Guids, Guids, lovely Guids, wonderful Guids....

Ah, I know I should do development in Python instead ...

procyon112 · 2006-01-24 Reply Admin

Didn't WoTC ban Lich because of stuff like this?

Jeff S · 2006-01-24 Reply Admin

OneFactor:
Yeah, I looked at that after making my post. It wasn't clear to me that any of the three options was robust. For Scope_Identity() it had a warning about triggers in the insert which would mean that it would bust if someone started created insert triggers. @@Identity and ident_current('table_name) seemed like it was rather "global" and vulnerable to concurrency issues if another transaction did an insert during the "lag time" between the original insert and the call to the identify variable or function. Perhaps I read the documentation wrong but my fears were not soothed by what I read.

You are correct about @@identity and ident_current(), but not about scope_identity:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/tsqlref/ts_sa-ses_6n8p.asp

Scope_Identity() should always be used since it will alwasy return the identity from the correct scope even if triggers occur, but it wasn't available in SQL versions < 2000.

Jeff S · 2006-01-24 Reply Admin

Some of the posts here mention doing the old "Count()+1" or "Max(ID)+1" techniques to get a new ID, but this @@ROWCOUNT method is even more ingenius because not only is it often wrong like the others, it is also exponentially less efficient!

2006-01-24 Reply Admin

Alex Papadimoulis:

<font color="#000099">SET</font> @intUserId = <font color="#000099">@@ROWCOUNT</font>

Maybe they just wanted each user to feel like he was #1

mrsticks1982 · 2006-01-24 Reply Admin

The code that surround it was from my last project .. oh wait that was written in oracle, Hated it!!! I am glad I left for greener pastures!!!!

Run Run Run!!!

2006-01-24 Reply Admin

I dunno, without looking at surrounding code, that could just be a typo (@@ROWCOUNT instead of @@IDENTITY). Which is bad, but if it's unintentional, I don't think it qualifies.

Unlike this little morsel of evil I inherited, which obviously took some thought and a certain degree of skill:

SELECT ...
FROM ITINERARYITEMS AS I
JOIN HOTELS AS H
ON CONVERT (
 INT
 , SUBSTRING(
 CONVERT (VARCHAR, I.Data)
 , PATINDEX('%<var name=''id''><string>%', I.Data) + 23
 , PATINDEX(
 '%</string>%'
 , SUBSTRING(
 CONVERT (VARCHAR, I.Data)
 , PATINDEX('%<var name=''id''><string>%', I.Data) + 23
 , 20
 )
 ) - 1
 )
 ) = H.ID
...
WHERE BookingType = 'hotel'
AND I.STATE=1
AND PATINDEX('%<var name=''id''><string>%', Data) > 0
AND TIME BETWEEN @StartDate AND @EndDate

(names anonymized a bit and formatted so you can see what it's doing. The original was all on one line, naturally)

Yes, it's joining between an int column in one table and a number inside some XML in a text column in another! (and that number is stored in a <string> tag!?)

OneFactor · 2006-01-24 Reply Admin

Satanicpuppy:
OneFactor:

Ytram:
My guess is that it was some kind of crappy manual identity field management. Kinda like handling an identity by using MAX(IdField) + 1.

That was my guess too. I must confess that I've done crap like max(id)+1 or select count(*)+1 from blah when needing to do ad hoc inserts to a table that has a primary key field int rather than identity. It doesn't seem too bad because it was done on the dev database where concurrency is not expected and when we migrate to production, we shut down the applications first the the DBA runs the diff script generated by Adept SQL.

I'm guessing the context is some dinky admin application to allow a single admin to insert a new user and assign an id. This doesnt sound like it is necessarily that bad. What do others think, is it really that bad?

yes, no, unknown, not specified, null, filenotfound, aSuffusionOfYellow

If you're inserting a user into a database, it is always always always better to let the database figure out what number to use, through whatever auto-incrementing key you've setup to take care of the user table. That is the only way you can make sure it's unique.

I had a situation once, where I'd run a bunch of bug testing and added a bunch of users, then removed them, so the counter was set at a higher number than the last existing row. So some bright boy comes along and decides that he's too smart to add stuff to the table using the methods that were set up for that...he's going to write a query to dump all his new users directly into the tables. So he figures out the last entry, and inserts all his users, setting their FOREIGN KEYS in related tables to what he imagines their ID numbers will be. Then, when regular users get added, we get duplicate foreign keys. Total snafu.

Stuff like that is very easy to do with a database. Use built-in methods wherever possible.

In my case, the parent table did not have an autonum or a built-in method. The table was expected "never to change". I may have even committed the crime of trying to derive what the foreign keys on child tables should be. Guids made all the problems go away.

2006-01-24 Reply Admin

YEAH! I'm FIRST!

I'm not, am I? Ahh well, I came pretty close. Why do some morons keep posting that? A lot of them aren't even first because people actually beat them to it.
And what's so great about being first, anyway? Do you really think you get any honour of being The First of one of the many posts on this site? What is it?
All I can say is it seems most people find it quite irritating. And if you wan't honour, then posting "FIRST" is probably the thing not to do.
Sorry, I, for one, am pretty irritated by all those "FIRST" posts (which are usually second posts).

jvancil · 2006-01-24 Reply Admin

Rick:
Its obvious that the 'designer' wanted to save the unnecessary space that would be used up by a UserId column in the table. You can always determine the UserId with the code below.
select * from USER_TABLE where UserName < "JOE BLOW"
SET @intUserId = @@ROWCOUNT

I consider this to be a very 'normal' way of doing things.

But... presumably this would always return 1... assuming there was only one "JOE BLOW" in the database... The only place this makes sense is if you are adding a new user to the end of the table and.... no wait. IT NEVER MAKES SENSE![H]

2006-01-24 Reply Admin

Let's try that again...

SELECT ...
FROM ITINERARYITEMS AS I
JOIN HOTELS AS H
ON CONVERT (
          INT
        , SUBSTRING(
              CONVERT (VARCHAR, I.Data)
            , PATINDEX('%<var name=''id''><string>%', I.Data) + 23
            , PATINDEX(
                  '%</string>%'
                , SUBSTRING(
                      CONVERT (VARCHAR, I.Data)
                    , PATINDEX('%<var name=''id''><string>%', I.Data) + 23
                    , 20
                )
            ) - 1
        )
    ) = H.ID 
...
WHERE BookingType = 'hotel'
AND I.STATE=1
AND PATINDEX('%<var name=''id''><string>%', Data) > 0
AND TIME BETWEEN @StartDate AND @EndDate

And that last comment was supposed to be "and the number is in a <string> tag!?"

2006-01-24 Reply Admin

OK, I give up.

jvancil · 2006-01-24 Reply Admin

Hugo:

OneFactor:
how to reliably determine what the autonum generated for the inserted record. I seem to recall it wasn't always obvious how to get that number (in fact, I do not know how to do it and would appreciate very much if someone could enlighten me).

Hi OneFactor,

Since an autonumber (or IDENTITY, as SQL Server calls it) should never be the only key for a table, the logical way is to read back the record using the business key to get the generated identity value.

Alternatives are SCOPE_IDENTITY(), @@IDENTITY, and IDENT_CURRENT('table_name'). They are described in Books Online.

Best, Hugo -trying hard to forget today's WTF code...

Why on earth would you submit that an IDENTITY should never be the only key for a table? That is (IMHO) assinine!

OneFactor · 2006-01-24 Reply Admin

procyon112:
Didn't WoTC ban Lich because of stuff like this?

I thought it was Word of Command. I think the term was "mystifier" rather than "spoiler" which never got banned. I'm still sore over losing a two out of three match to a channel-lotus-mountain-fireball first round combo twice in a row. In hindsight I should have separated the Channeling from the Lotus when I shuffled the deck.

Though I suppose he lost more than I did as someone (not me) stole the Lotus when he wasn't looking.

2006-01-24 Reply Admin

Anonymous:
YEAH! I'm FIRST!

I'm not, am I? Ahh well, I came pretty close. Why do some morons keep posting that? A lot of them aren't even first because people actually beat them to it.
And what's so great about being first, anyway? Do you really think you get any honour of being The First of one of the many posts on this site? What is it?
All I can say is it seems most people find it quite irritating. And if you wan't honour, then posting "FIRST" is probably the thing not to do.
Sorry, I, for one, am pretty irritated by all those "FIRST" posts (which are usually second posts).

At least you were the first one to complain. You win the kewpie doll. If there were more prizes available, perhaps the First Post winner could win something.

2006-01-24 Reply Admin

Jeff S:

OneFactor:
Yeah, I looked at that after making my post. It wasn't clear to me that any of the three options was robust. For Scope_Identity() it had a warning about triggers in the insert which would mean that it would bust if someone started created insert triggers. @@Identity and ident_current('table_name) seemed like it was rather "global" and vulnerable to concurrency issues if another transaction did an insert during the "lag time" between the original insert and the call to the identify variable or function. Perhaps I read the documentation wrong but my fears were not soothed by what I read.

You are correct about @@identity and ident_current(), but not about scope_identity:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/tsqlref/ts_sa-ses_6n8p.asp

Scope_Identity() should always be used since it will alwasy return the identity from the correct scope even if triggers occur, but it wasn't available in SQL versions < 2000.

Why is it that nobody ever thought of having the generated ID returned as a parameter of the execute statement that executed the sql statement?

That is exactly because of this problem that we settled down for a table with an ID count. We set a lock on the table before picking up the number and release the hold once updated with the new value to prevent concurrency. The GetNewID procedure will retry to get a number for a certain time before giving up if the record is locked by another process.

OneFactor · 2006-01-24 Reply Admin

Jeff S:

OneFactor:
Yeah, I looked at that after making my post. It wasn't clear to me that any of the three options was robust. For Scope_Identity() it had a warning about triggers in the insert which would mean that it would bust if someone started created insert triggers. @@Identity and ident_current('table_name) seemed like it was rather "global" and vulnerable to concurrency issues if another transaction did an insert during the "lag time" between the original insert and the call to the identify variable or function. Perhaps I read the documentation wrong but my fears were not soothed by what I read.

You are correct about @@identity and ident_current(), but not about scope_identity:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/tsqlref/ts_sa-ses_6n8p.asp

Scope_Identity() should always be used since it will alwasy return the identity from the correct scope even if triggers occur, but it wasn't available in SQL versions < 2000.

Let me see if I got this straight: @@identity runs into trigger trouble, ident_current runs into concurrency issues, scope_identity does not work before sql server 2000. In that case, this @@rowcount garbage may be a result of a poor sap being stuck on an older Sql Server and not having a good solution available.

I guess I have another good interview question for supposed SQL experts now.

CornedBee · 2006-01-24 Reply Admin

Doesn't SQL server have sequences?

SELECT curval('my_id_sequence) as newid;

Now the value of newid is mine, all mine. Nobody else will ever get it! (Until the sequences wraps around, anyway.)

2006-01-24 Reply Admin

IF (<font>@@ROWCOUNT = 1)
BEGIN
  PRINT "First Post!"
END
ELSE
  PRINT "Last Post!!!"
 
</font>

2006-01-24 Reply Admin

SELECT curval('my_id_sequence) as newid;

Wouldn't that be "nextval"?

ammoQ · 2006-01-24 Reply Admin

Jeff S:
Some of the posts here mention doing the old "Count()+1" or "Max(ID)+1" techniques to get a new ID, but this @@ROWCOUNT method is even more ingenius because not only is it often wrong like the others, it is also exponentially less efficient!

Why? Isn't it basically the same like count()? I aggree that max(ID) is much faster if ID is indexed, a reasonable though unsafe (considering the circumstances) assumption.

Jeff S · 2006-01-24 Reply Admin

ammoQ:
Jeff S:

Some of the posts here mention doing the old "Count()+1" or "Max(ID)+1" techniques to get a new ID, but this @@ROWCOUNT method is even more ingenius because not only is it often wrong like the others, it is also exponentially less efficient!

Why? Isn't it basically the same like count()? I aggree that max(ID) is much faster if ID is indexed, a reasonable though unsafe (considering the circumstances) assumption.

Which do you think is a more efficient way to return the # of rows in a table to the client?

Option 1: SELECT COUNT(*) FROM Table

Option 2: SELECT * FROM Table; SELECT @@ROWCOUNT

Maurits · 2006-01-24 Reply Admin

Anonymous:

IF (<font>@@ROWCOUNT = 1)
BEGIN
  PRINT "First Post!"
END
ELSE
  PRINT "Last Post!!!"
 
</font>

Take out the ELSE and I like it :)

2006-01-24 Reply Admin

Hugo:
Since an autonumber (or IDENTITY, as SQL Server calls it) should never be the only key for a table, the logical way is to read back the record using the business key to get the generated identity value.

Why? I guess I am a newb, but I have done this in situations where there were no other obvious columns to use as a primary key, and I have never run into any problems.

Jeff S · 2006-01-24 Reply Admin

jvancil:
Hugo:

OneFactor:
how to reliably determine what the autonum generated for the inserted record. I seem to recall it wasn't always obvious how to get that number (in fact, I do not know how to do it and would appreciate very much if someone could enlighten me).

Hi OneFactor,

Since an autonumber (or IDENTITY, as SQL Server calls it) should never be the only key for a table, the logical way is to read back the record using the business key to get the generated identity value.

Alternatives are SCOPE_IDENTITY(), @@IDENTITY, and IDENT_CURRENT('table_name'). They are described in Books Online.

Best, Hugo -trying hard to forget today's WTF code...

Why on earth would you submit that an IDENTITY should never be the only key for a table? That is (IMHO) assinine!

yeah, I agree, data integrity is assinine !

A Representative Sample

Leave a comment on “A Representative Sample”