• Anders Hesselbom (unregistered)

    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.

  • Tarq (unregistered)

    WTF

    On error resume stupid.

  • BradC (cs)

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

  • Anders Hesselbom (unregistered) in reply to BradC
    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!!!)<o:p></o:p>

  • asmodai (cs)

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

  • Daniel T (unregistered) in reply to Tarq

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

  • emptyset (cs)

    Alex Papadimoulis:

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

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

  • Alexis de Torquemada (cs) in reply to BradC
    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.

  • Dave (unregistered)

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

  • asmodai (cs) in reply to Dave
    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.
  • travis (unregistered) in reply to Dave

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

  • Henry Troup (unregistered)

    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?

  • Anonymous Bastard (unregistered)

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

  • wkempf (unregistered) in reply to Anonymous Bastard
    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#.
  • Jeff S (cs)

    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?

  • travis (unregistered) in reply to Anonymous Bastard

    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

  • Abby Normal (unregistered)

    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.

  • nobody (unregistered) in reply to Abby Normal

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

  • DerelictMan (unregistered) in reply to Dave
    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. :)

  • FranksAndBeans (unregistered)

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

  • OneFactor (cs) in reply to Abby Normal

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

  • OneFactor (cs) in reply to OneFactor

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

  • DerelictMan (unregistered) in reply to OneFactor
    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...
  • suidae (unregistered) in reply to DerelictMan
    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.
  • Colin (unregistered)

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

    Look Pa! I got one!


  • wkempf (unregistered) in reply to OneFactor
    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.
  • spoilsportmotors (unregistered) in reply to wkempf

    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.

  • Dave (unregistered)

    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.

  • christoofar (unregistered) in reply to OneFactor

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

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

  • OneFactor (cs) in reply to spoilsportmotors

    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

  • ammoQ (cs)

    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?

  • WaterBreath (cs)

    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.

  • bhousel (unregistered) in reply to WaterBreath

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

  • DerelictMan (unregistered) in reply to bhousel
    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...
  • Jon (unregistered) in reply to OneFactor
    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.
  • jeremydwill (cs) in reply to bhousel
    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.

  • OneFactor (cs) in reply to Jon

    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 (cs) in reply to jeremydwill
    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 <VALUES Insert To Parameters or>
        WHERE NOT EXISTS (SELECT <UNIQUE Columns>FROM
    WHERE <UNIQUE Columns>= <UNIQUE Values>) Or *relevant*, whichever you prefer.
  • ammoQ (cs) in reply to WaterBreath
    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.
  • DS (unregistered) in reply to nobody
    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..."))
  • WaterBreath (cs) in reply to jeremydwill

    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?

  • WaterBreath (cs) in reply to ammoQ
    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.  ;)
  • ammoQ (cs) in reply to bhousel
    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.
  • Dustin (cs) in reply to WaterBreath

    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.

  • Kaizer (cs) in reply to travis
    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:

    <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

  • Malyon (unregistered)
    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;
    }

     

  • jeremydwill (cs) in reply to WaterBreath

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

  • RevMike (unregistered) in reply to ammoQ
    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.

  • qiguai (unregistered) in reply to Dave
    Anonymous:
    <some_value>.

    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.
    </some_value>
  • qiguai (unregistered) in reply to qiguai

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

Leave a comment on “Exceptional Error Handling ”

Log In or post as a guest

Replying to comment #:

« Return to Article