Comment On Exceptional Error Handling

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.

WTF

On error resume stupid.

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! ;)

BradC:

Looks good to me! ;)

Yes, fine.
The golden rule of coding: Do not ever check the manual. Just do it

(I can't
handle this kind of posts!!!)

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. ;)

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...

Alex Papadimoulis:

    lblError.Text =
      "This code is already associated with another "
      + "Project, please select another code.";

old_manv("good, riki.  goooood.  ha ha ha.  ha ha ha.") ;

BradC:

Come on, looks like you're dealing with a bullet-proof application, one so robust, that it can ONLY ever encounter one error.

If that were so, the if-statement would be redundant.

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...

Anonymous:

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)....

Python does for example, TypeError, KeyError, etc.

Anonymous:

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...

yes there is...and thats what the WTF is...

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?

..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.

Anonymous:

..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.

Which is why you should assume it's not Java.  My guess is C#.

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?

Anonymous:

..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.

Looked like C# to me

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.

if this is java, another mistake is using the "==" operator instead of .equals()

Anonymous:

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...

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. :)

And how many unit tests did it take to just get the line endings right?  Correct punctuation?  Correct case?

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(]

BTW, this is C# not Java as evidenced by the lonely "throw" with nothing to separate it from the semicolon. [H]

OneFactor:

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.

Well put.  You managed to sum that up much more succinctly than I did...

Anonymous:

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.

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.

I wonder if his father taught him to catch files with a shotgun?



Look Pa! I got one!

OneFactor:

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(]

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.

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.

Sounds like there should just be some kind of validation first 
like perhaps using some SQL to perform a count where the ProjectIDCode
= 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.

If this is C# code, == versus .Equals() advocates should look here:



http://www.stevex.org/cs/blogs/dottext/archive/2004/04/20/376.aspx

Anonymous:

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.

I wonder a similar property exists on IOException... Guess I'll go ask Google

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?

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.

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.

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...

Anonymous:

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...

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...

OneFactor:

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.

The code
could have used SqlException.Number, which is less of a WTF.

Anonymous:

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.

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...

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.

Anonymous:

OneFactor:

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.

The code could have used SqlException.Number, which is less of a WTF.

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"?

jeremydwill:

...(modify to your SQL's dialect - I know this pseudo-code works on SQL Server 2000 after relavent pieces are filled in):

INSERT INTO

SELECT
    WHERE NOT EXISTS (SELECT FROM

WHERE = )

Or *relevant*, whichever you prefer.

Re: Exceptional Error Handling 2005-09-26 18:44 • by ammoQ

45270 in reply to 45257 Reply Quote

WaterBreath: 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. 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.

Re: Exceptional Error Handling 2005-09-26 18:44 • by DS

45271 in reply to 45232 Reply Quote

Anonymous: if this is java, another mistake is using the "==" operator instead of .equals() Actually the correct method would be:  isTrue(isEqual(e.Message, "Cannot insert duplicate key row in object..."))

Re: Exceptional Error Handling 2005-09-26 18:44 • by WaterBreath

45272 in reply to 45267 Reply Quote

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?

Re: Exceptional Error Handling 2005-09-26 18:46 • by WaterBreath

45273 in reply to 45270 Reply Quote

ammoQ: 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. Then, IMHO, it doesn't qualify as a simple write.  ;)

Re: Exceptional Error Handling 2005-09-26 18:51 • by ammoQ

45274 in reply to 45258 Reply Quote

Anonymous: 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... 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.

Re: Exceptional Error Handling 2005-09-26 18:59 • by Dustin

45275 in reply to 45273 Reply Quote

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.

Re: Exceptional Error Handling 2005-09-26 19:51 • by Kaizer

45276 in reply to 45230 Reply Quote

Anonymous: Anonymous: ..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. Looked like C# to me   Yes, it's C#. And a good start to fixing the cluster-f would be catching a SqlException: catch (Sytstem.Data.SqlClient.SqlException ex) before a generic exception.  Then checking the ex.Number. One has to wonder how much code was in the try block, considering only 1 line of it would throw that error.  WTF

Re: Exceptional Error Handling 2005-09-26 20:22 • by Malyon

45279 Reply Quote

Alex Papadimoulis:     lblError.Text =       "This code is already associated with another "       + "Project, please select another code.";     return; 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; }

Re: Exceptional Error Handling 2005-09-26 21:04 • by jeremydwill

45280 in reply to 45272 Reply Quote

WaterBreath: 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? 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).

Re: Exceptional Error Handling 2005-09-26 21:18 • by RevMike

45281 in reply to 45274 Reply Quote

ammoQ: IMO it's better to let the database run into the error, catch it and process the result. 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. ammoQ: Repeating the work of the database in the program is a bad idea in most cases. That is a mantra that should be repeated by junior developers every day.

Re: Exceptional Error Handling 2005-09-26 22:21 • by qiguai

45286 in reply to 45246 Reply Quote

Anonymous: . 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. 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.

Re: Exceptional Error Handling 2005-09-26 22:22 • by qiguai

45287 in reply to 45286 Reply Quote

Not that I'm defending this code....

« Prev

Page 1 | Page 2

Next »

Add Comment