Correct error-handling practices for the data-layer.

Question

What are good things to check for, with respect to error-handling, when you are dealing with the data-access-layer? For example, let's assume I have this function..

    Public Function UserExists(ByVal userName As String) As DataTable
        Dim dt As Object = Nothing
        Dim arSqlParameters(0) As SqlParameter
        arSqlParameters(0) = New SqlParameter("@UserName", SqlDbType.NVarChar, 50)
        arSqlParameters(0).value = userName
        dt = ABC.APP.DAL.DALHelper.ExecuteDatatable(ConnectionString, CommandType.StoredProcedure, "dbo.aspnet_sprGetUserByUsername", arSqlParameters)
        Return dt
    End Function

This seems like very lazy and unsafe coding. How would you go about ensuring that your code elegantly handles anything unexpected in a situation like this?

I'm new to vb.net and the app that I'm working on has no error handling, so I figured this would be the best place to look for advice. Thanks in advance :)

Answer 1

This might be of use to you...

.NET Framework Developer's Guide - Design Guidelines for Exceptions

Answer 2

Opinions are likely to vary wildly on a topic like this, but here's my take. Only try to deal with the exceptions that you relate to this area. That's a vague statement, but what I mean is, did the user pass a string with more chars than the column in the db. Or did they violate some other business rule.

I would not catch errors here that imply the database is down. Down this far in the code, You catch errors that you can deal with, and your app needs it's database. Declare a more global exception handler that logs the problem, notifies someone, whatever... and present the user with a "graceful" exit.

I don't see any value in catching in each method a problem with the database. You're just repeating code for a scenario that could bomb any part of your datalayer. That being said, if you choose to catch db errors (or other general errors) in this method, at least set the innerexception to the caught exception if you throw a new exception. Or better, log what you want and then just "throw", not "throw ex". It will keep the original stack trace.

Again, you will get a lot of varying thoughts on this, and there is no clear right and wrong, just preferences.

Answer 3

looking inside Open Source ORM solutions' code (Subsonic, nHibernate) will help you.

By my limited knowledge I think error related to connections, connection timeouts etc. should be passed as such because DAL cannot do much about it. Error related to the validations (like field lengths, datatypes) etc should be returned with appropriate details. You may provide validation methods (Subsonic contains it) that validate the field values and returns the appropriate error details.

Answer 4

Not sure why you're not declaring dt as a datatable - what is the motiviation for "dim dt as object = nothing"?

Really the only line that can reasonably fail is the "dt=ABC.APP.DAL...." call.

In that line, you could have a few errors:

• The stored procedure or parameter names are wrong. This should be caught at design-time (not by a built-in checking mechanism, but the first time that you try to run the code)
• An error occurs in the stored procedure. Sprocs use deferred name resolution, which can lead to runtime errors if (for instance) objects don't exist at the time that the sproc is called. Again, this is most likely to rear it's head in testing.
• A deadlock. In this case, you should catch and resubmit the batch. Here's an intro and set of links on handling SQL errors in application code.
• A parameter that is passed is invalid (too long, wrong datatype, etc). This should be checked before you call the sproc.

Answer 5

I've put a lot of thought into this as well. A lot depends on the particular database, so depending on if you are using SQL2000 or SQL2005+ you will either be doing return codes, RAISERROR or TRY-CATCH in the stored procedure.

RAISERROR is preferably to return codes, because developers using the stored procedure get more information. Optimally you'd pick numbers 50000 and up and register the corresponding messages, but this is a lot of work just to communicate what can be communicated in the text of the RAISERROR call

The return code is uninterperatable without reading the source code of the stored procedure. a return value of 0 might be success, failure, 0 rows affected, the just generated primary key, or that no return code was set depending on the whims of the stored procedure writer that day. Also, ADO.NET returns row count from ExecuteNonQuery, so some developers will find it hard to quickly discover the return code.

Ad hoc solutions are also bad, eg.

IF @@Error>0 
   SELECT 'Something bad happened'

If you have the option of using CLR stored procedures, then C# style error handling comes into play.

So add RAISERROR to your stored procedures, then catch SqlException, report to the user anything that really is a user data entry Validation, log anything that is a developer abusing the API, and maybe attempt a re-try for connection or execution timeouts.

IF @Foo =42
        BEGIN
            RAISERROR ( 'Paramater @foo can''t be 42',
                16, -- Severity = Misc. User Error.
                1 -- State. == ad hoc way of finding line of code that raised error
               ) ;  
      RETURN -1; --Just because some developers are looking for this by habit
        END