- 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
Perhaps I have managed exceptions the wrong way... thanks for showing how it's done!
Perhaps this is easier than testing the class of the exception.
Admin
WTF
On error resume stupid.
Admin
Come on, looks like you're dealing with a bullet-proof application, one so robust, that it can ONLY ever encounter one error.
Looks good to me! ;)
Admin
Yes, fine. The golden rule of coding: Do not ever check the manual. Just do it
(I can't handle this kind of posts!!!)<o:p></o:p>
Admin
What scares me more is that the try {} had to be snipped back for brevity. :|
At least this guy/girl is programming defensively! Too many don't even check return values or the likes at all. ;)
Admin
Wow - is there possibly a worse way to do this? And what is this crap even trying to do (i.e. "This code is already associated with another Project, please select another code.")?
--Daniel T
p.s. It may have been the library guy's fault if the DB calls only throw generic exceptions. In that case it would be hard to get the type...
Admin
<FONT face="Courier New" size=2>old_manv("good, riki. goooood. </FONT><FONT face="Courier New" size=2>ha ha ha. ha ha ha.") ;</FONT>
Admin
If that were so, the if-statement would be redundant.
Admin
First off, isn't there a way to catch a specific exception? It always makes me cringe when somebody catches Exception or Throwable (in Java that is)....
Second of all, this looks like the app that this code is part of allows a user to enter some value that is part of a primary key. WTF? Smells like the work of an inexperienced, offshore hack...
Admin
Python does for example, TypeError, KeyError, etc.
Admin
yes there is...and thats what the WTF is...
Admin
Of course, this can only work if the database language is English ... and for exactly one database, and maybe only for certain releases.
(And, sad to say, there have been instances where I've had to resort to this. Not many, and it's a last resort. But sometimes the error has been through too many layers or reinterpreted or whatever.)
IMO, error handling is not often enough addresses in architectural discussions. How many times does only an HRESULT come back, and the level that should handle the error is left guessing about what the error was?
Admin
..and there's a certain possibility that the error handling's gonna go wrong as well. Last I checked, you don't wanna compare strings that way in Java.
Admin
Which is why you should assume it's not Java. My guess is C#.
Admin
The WTF could be in the code seen here, and/or it could be in the class library being used ... if that class only throws generic Exceptions and never those of a particular type.
And, no, checking for just 1 possible exception is not a WTF -- we've all written code that is outside of a try/catch block, right?
Admin
Looked like C# to me
Admin
I've been following wtf for a few months now, and it's really great. The coding tips are very helpful, for example I would never have thought of this myself. It really shows me the expert programming techniques, and I always try to apply the same techniques in my own projects. Thanks!
PS: My biggest dream is to have my own code mentioned on wtf, but I fear the competition is stiff.
Admin
if this is java, another mistake is using the "==" operator instead of .equals()
Admin
I didn't write this code, but I'm going to defend at least part of it (no comment on catching Exception instead of SqlException).
If you'll look at the text of the error the app is checking for, you'll see it references a "unique index". You can't assume that this is the primary key of the table. There are lots of cases where one would want a unique index on a field that isn't the primary key.
For example, suppose you have a table for users that has "id" as the primary, surrogate key, and "username" as the login name. You want to make sure that login names are unique in your system, so you create a unique index on that field so that the database ensures you never have two accounts with the same username. So far so good.
Let's say you have a web application that has a registration form where a user can pick their username. You obviously want to trap for the case where a user enters a username that is already taken. What's the best way to do this? Should you execute a separate select query ("select count(username) from users where ...") and then attempt to insert the new record only if the select didn't find any rows? Lots of people take this approach, but this introduces a race condition between the select query and the insert query. It's possible that the row doesn't exist when you do your select, but it DOES exist before you do your insert. Unlikely, but possible.
Sure, you could ensure isolation between your selects an inserts by wrapping it in a transaction (or locking the table, etc. depending on the capabilities of your RDBMS), but isn't it much simpler to let the unique index you defined do your work for you? Simply attempt the insert, and trap the error that is generated. If it references your unique index, then you know you've got the case of username that has already been selected. Why go to all the trouble of verifying uniqueness when the database will do that for you already? It's the classic example of Don't Repeat Yourself.
If you're going to take this general approach, I'm aware of no better way to check for specific errors (that reference specific index names, etc.) than to do SOME sort of parsing on the error message that is returned from the SQL layer (if I'm being stupid and there is some better way someone please inform me, I'm not a Java person but I have used this general pattern in other languages). Of course, I don't think it's necessary to check the ENTIRE sql error string...in this case I would simply see if the error string contains "adx_Projects_ProjectIDCode" since the only error that is likely to reference that particular unique index name is the case of a key violation.
Perhaps catching Exception instead of SqlException is a WTF, or perhaps checking for the ENTIRE error text (newlines and everything) is quite a WTF (it is), but I for one happen to think the general approach (catching the SqlException and checking to see if the error text references your key) is quite sound. You may now commence flaming. :)
Admin
And how many unit tests did it take to just get the line endings right? Correct punctuation? Correct case?
Admin
All right, I guess I'm gonna become defender of the WTF today.
Yeah it stinks, but assuming this is C# and SQL Server there is not much you can do to improve on this. If there is, please show me how and I will thank you profusely as I want very much to be proven a nincompoop on this one.
As much as you may wish to check constraints before writing something, there always exists the possibility that the constraint was fine when you checked and then isn't when you actually go and do the change. And if constraint errors are unavoidable, then a little experience with C# reveals that they all have the exact same type: SQLException and the only way to pick out this condition is to test the message.
This problem also happens when you want a handle on a file and want to move on to a new filename (next in sequence) if you cannot get a write lock and all you get is a generic IOException. C# exceptions are way too broadly-typed to use typed catches.
I've read the complaints and agree with them all but noone has suggested a better idea. If that is the case, then the WTF is really in the language. [+o(]
Admin
BTW, this is C# not Java as evidenced by the lonely "throw" with nothing to separate it from the semicolon. [H]
Admin
Well put. You managed to sum that up much more succinctly than I did...
Admin
I'd agree that checking the text of the exception for the key name might be valid, but I wouldn't do it in GUI code like this. That kind of logic belongs down in the database access layer, where an explicit error can be generated. That would be caught by the business rules layer, and then the appropriate GUI response would follow.
This appears to be checking the text of the DB error directly in the user interface.
Admin
I wonder if his father taught him to catch files with a shotgun?
Look Pa! I got one!
Admin
Nope, it's with the code. Assuming C#, the SqlException class gives you enough detail (see the Number property) to determine they type of error, and then a simple search for the column name would suffice. Even then, though, the string being searched for should not be hard coded, as indicated by the wtf description.
Admin
If you poke through the documentation - heresy, I know - you'll see that the number corresponds to values from master.dbo.sysmessages
In this case, the error is presumably number 2601.
Hard-coding these values feels pretty icky too, but you can at least make a wrapper class to handle the cases you care about and expose the constants as meaningful named, er, constants.
Admin
Sounds like there should just be some kind of validation first like perhaps using some SQL to perform a count where the ProjectIDCode = <SOME_VALUE> or something like that....
I just think it's poor design when you rely on the DB to throw an exception with a specific message as part of the validation process.
Admin
If this is C# code, == versus .Equals() advocates should look here:
http://www.stevex.org/cs/blogs/dottext/archive/2004/04/20/376.aspx
Admin
I wonder a similar property exists on IOException... Guess I'll go ask Google
Admin
This looks like a quick&dirty hack that replaces a generic error-popup with a reasonable message for the user. Comparing the text is a bit of a WTF; on the other hand, it does the job and won't break at least until the database is upgraded (which requires thorough testing anyway). Comparing the error number would be a shortcut but without parsing the text this might lead to wrong results; the duplicate key might have happened in another table or for another column, thus annoying the user with incorrect error messages. Is this the right place to catch the exception? I can hardly tell from this sniplet. Catching Exception instead of SQLException: Who knows which kind of exception the helper functions throw?
Admin
A couple of people have hit on a few different issues that are central here.
The people who criticized the idea of pre-checking due to potential race conditions are right. It's a legitimate concern, especially in large systems. But if you'd rather deal with the collision possibility as preemptively as possible, there are ways to do it that aren't as kludgy and brittle as what this guy did.
Databases such as SQL Server and Oracle have error codes which are generally available in some way to the calling program. If nothing else, the program should examine this error code. To make this as reliable and unambiguous as possible, the "try" should be wrapped as closely as possible around the write attempt.
So then after examining the error code, you know the failure was a unique constraint violation. But before you can report back to the user, you need to find out what data caused it. This is not justification to go parsing error messages, let alone comparing them to hard coded values...
On a simple write, it will probably be safe to assume you know which column caused the unique constraint violation and just report that field as the offender. However, if the row has multiple unique columns constraints, then you can query the table again after the failed write to determine which column it was that had a collision, and then report the offending field(s). And if this becomes prohibitively complex due to the number of columns or constraints on the row... well, then you're a victim of poor database architecture on top of everything else going on here.
Yes, it's a fair amount of extra forethought and implementative effort. But there's big ROI.
Admin
The more correct way to do this is to begin a transaction, perform your select with an table lock. If the row is not there, perform the insert. Finally commit the transaction and return the rowid. (This is assuming that adding users is something that happens infrequently and quickly enough that you can briefly lock the table). Real databases have these features so that race conditions aren't really a problem if you know what you're doing...
Admin
The database is already going to obtain an exclusive lock on the unique index whenever the insert is performed, which should only block other writers that are touching the field that the unique index references (by either inserting a whole row, or updating that field). Unless I'm mistaken (and correct me if I am), your approach will block ALL writers until you commit/rollback your transaction. It seems to me that allowing the unique index to be the gatekeeper for updates is far more scalable as the number of hits on your table increases. I'd prefer moving the error parsing into a low-level, centralized location that can keep the key name => exception type mapping in one place and easily updated vs. getting into a general habit of starting transactions with high isolation levels.
Bottom line is at some point in your application you are going to have to reference specific table names/field names, and yes, sometimes even index names. Trying to jump through hoops to avoid that seems silly to me. As long as you try to contain the schema-dependent code in as small an area as possible, I don't see the issue...
Admin
Admin
I am not sure that a table lock is even necessary. You can easily make the INSERT statement itself conditional, such that nothing is inserted if the row already exists (modify to your SQL's dialect - I know this pseudo-code works on SQL Server 2000 after relavent pieces are filled in):
INSERT INTO <Table Name>
SELECT <Values or Parameters To Insert>
WHERE NOT EXISTS (SELECT <Unique Columns> FROM <Table Name> WHERE <Unique Columns> = <Unique Values>)
You can tell if the INSERT worked by checking the number of rows affected. Eliminates unique constraint errors.
Admin
Thanks for the tip on SqlException.Number though people are right to point out that you need to parse the message for table and field names anyways. "Less of a WTF"? Is that like being "kind of pregnant" or "semi-virgin"?
Admin
Admin
If your database systems supports triggers, a simple write (insert or update) may cause (possibly lots of) writes to other tables; for that reason, it's not safe to assume to know which column (or even table) caused the problem.
Admin
Actually the correct method would be:
isTrue(isEqual(e.Message, "Cannot insert duplicate key row in object..."))
Admin
Definitely an alternative to using a try-catch block. But then you're back to actively passing around success/failure indicators (assuming you're calling stored code), which many will argue should be avoided. Although, since such behavior could be restricted to happen only at the app/DB interface, it may not be such a terrible thing. Or there's always the in-DB error log option, though it's a bit more cumbersome for recovering from non-critical failures like this one.
Anyone have any ideas on good ways to communicate the nature of the failure back to the app if we use jeremydwill's suggestion?
Admin
Then, IMHO, it doesn't qualify as a simple write. ;)
Admin
This means loosing performance and scalability as well as repeating yourself: the unique constraint is defined and enforced both in the database and in your program. (Guess what happens when someone decides that the unique constraint is no longer necessary and drops it from the database, also dropping the index). IMO it's better to let the database run into the error, catch it and process the result.
Repeating the work of the database in the program is a bad idea in most cases.
Admin
It just doesn't seem like there's any reason to throw an exception, or even catch that particular exception here. The programmer obviously knew that an error would occur so why not handle it at the database level with a transaction? I haven't been programming for a long time so correct me if I'm wrong, but it just seems like exceptions should be used for unforseen errors and that the errors you know are going to happen should be handled accordingly.
Admin
Yes, it's C#.
And a good start to fixing the cluster-f would be catching a SqlException:
<FONT color=#0000ff>catch (Sytstem.Data.SqlClient.SqlException ex)</FONT>
before a generic exception. Then checking the <FONT color=#0000ff>ex.Number.</FONT>
<FONT color=#000000>One has to wonder how much code was in the try block, considering only 1 line of it would throw that error. </FONT>
WTF
Admin
Samples of other error reporting likely to be found in this program:
if (param1 < 0) {
lblError.Text = "Invalid parameter passed.";
return;
}
if (param1 < param2) {
lblError.Text = "You passed an invalid parameter.";
return;
}
if (param1 + 5 > param2) {
lblError.Text = "Interface protocol violation: invalid parameter.";
return;
}
if (param3 < param2 - 7) {
lblError.Text = "Yo, that ain't how you call this function, homie.";
return;
}
if (param3 > 100) {
lblError.Text = "No habla machina.";
return;
}
Admin
What I proposed defintitely will not work if multiple possible duplicate checks are used in the NOT EXISTS subquery. The most you can tell is "did the insert work or not".
My technique was based on the fact that only one unique constraint check is being performed in the original WTF. It does not appear that Alex snipped additional checks, so it looks like the developer was only concerned about one kind of duplication. I was simply showing an alternative that did avoided separate select/insert combinations, did not require parsing error text in the exception handling, and that did not require multiple trips to the DB (as someone else suggested).
If it is necessary to know why the INSERT failed, then the developer is forced to use error codes, output parameters, or return values (if you are using stored procedures of course).
Admin
I agree. There is no reason to lock the table here while executing several statements. It may seem trivial to hold a table lock across two sql statements, but one never knows when the process or thread executing this bit of code might be swapped out. Now other theads may attempt to access this table and continue blocking until this process is swapped back in to complete the transaction and release the lock. Now the small amount of time the table was locked gets multiplied. And this behaviour likely wouldn't show up until load testing if not production.
Catching SqlException and referencing the unique constraint violation via the ErrorNumber would turn this into a reasonable piece of code.
That is a mantra that should be repeated by junior developers every day.
Admin
Why? Have you considered that DB access might be quite expensive in some cases, and that there might be many constraints on the data? Or that the DB is designed to enforce certain types of constraints very efficiently? It's certainly bad design to push validation onto the DB in some cases, but it seems to me that it is good design in other cases.
</some_value>
Admin
Not that I'm defending this code....