Surely a more elegant way

In the code below where I have the first catch will the return false staement result in the Finally block not getting called? Or does the Finally block get called regardless??


Code: 
    Public Function Save() As Boolean

        Dim intX As Integer

         set internal error flag to false

        m_Error = False

         if validatedata returns false the data cannot be saved, user must recify

        If Not ValidateData() Then

            Return False

        End If

         if CalculateServiceDates returns false the data cannot be saved as service due dates are not created

        If Not CalculateServiceDates() Then

            Return False

        End If

         create a new command builder object based on this supplier objects data

        m_ocbContract = New System.Data.OleDb.OleDbCommandBuilder(m_odaContract)

         protect this section of code

        Try

             signal beginning of edit 

            m_drContract.BeginEdit()

             write the private members to the corresponding data column

            m_drContract.Item("pr_number") = m_PRNumber
            m_drContract.Item("budget") = m_BudgetCode
            m_drContract.Item("dept") = m_DepartmentCode
            m_drContract.Item("po_number") = m_PONumber
            m_drContract.Item("dated") = m_Dated
            m_drContract.Item("Commencement") = m_Commencement
            m_drContract.Item("expires") = m_Expiry
            m_drContract.Item("contact") = m_Contact
            m_drContract.Item("contact_tel") = m_ContactTelephone
            m_drContract.Item("price") = m_Cost
            m_drContract.Item("frequency") = m_Frequency
            m_drContract.Item("order_line") = m_OrderLine
            m_drContract.Item("description") = m_Detail
            m_drContract.Item("internal") = m_Internal

             cycle through array and store service due dates in table

            For intX = 0 To m_Frequency - 1

                m_drContract.Item("s" & intX) = m_ServiceDates(intX, 0)
                m_drContract.Item("s" & intX & "done") = m_ServiceDates(intX, 1)

            Next

             signal end of edit

            m_drContract.EndEdit()

             get the required update command for this data 

            m_odaContract.UpdateCommand = m_ocbContract.GetUpdateCommand

             update the table in the database checking for concurrency errors while we are at it

            m_odaContract.Update(m_dsContract.Tables("tblContracts").GetChanges)

             signal to accept the changes

            m_dsContract.Tables("tblContracts").AcceptChanges()

             close the connection

            m_odaContract.UpdateCommand.Connection.Close()

        Catch objConcurrencyError As DBConcurrencyException

            ShowError("Location:   Class Contract" & ControlChars.CrLf & ControlChars.CrLf & "Procedure:  Save() As " & _
                      "Boolean" & ControlChars.CrLf & ControlChars.CrLf & "Error Text: " & "Another user has made " & _
                      "changes to this record since you opened it.  You must " & ControlChars.CrLf & _
                      "reload the record and try again.")

             set internal error flag to true

            m_Error = True

            Return False

        Catch objException As Exception

            ShowError("Location:   Class Contract" & ControlChars.CrLf & ControlChars.CrLf & _
                      "Procedure:  Save() As Boolean" & ControlChars.CrLf & ControlChars.CrLf & _
                      "Error Text: " & objException.Message)

             set internal error flag to true

            m_Error = True

        Finally

            If m_odaContract.UpdateCommand.Connection.State.Open Then

                 close the connection

                m_odaContract.UpdateCommand.Connection.Close()

            End If

        End Try

         return true to sigan data has been saved

        Return True

    End Function
 
Derek you say change it to SQLException, would this mean then that any other form of error may be missed? I tend to use Exception in all my try blocks as a sort of catch all. Am I in the wrong here then?
Click to expand...
Generally speaking if your code is erroneous you should fix it, not hand over its problems to a Try/Catch block. Some errors can arise due to reasons out of our hands, and accessing MS SQL Server is one of them. That doesnt mean that all errors in that block should be handled however. In your case only exceptions of type SqlException should be handled, since all other errors are most likely the result of poorly written code.
 
Mmm point taken. However Im using an Access database, not SQL Server so I presume then there is an exception of type Access or something?
 
In addition to Dereks comment Id like to point out one possible exception:

All front-end / GUI user-event handlers should contain a try/catch/finally block. Never ever will a larger application be free of bugs. I believe it is absolutely unacceptable and way too risky to let the whole thing crash in such a case. The user must be allowed to continue working, perhaps retry his last steps or move over to another task. And, of course, there must always be a "controlled shutdown" of the application.
 
To hog: The SqlException is for use with the SqlClient namespace (for SQL Server). For the OleDb theres an OleDbException object.

-Nerseus
 
Thanks to you all. I am now confused as to the best approach regarding error trapping. My method to date has been as Heiko states, but Derek has good points too. So whos right and whos wrong?
 
Id pretty much always trap for Exception if theres even a remote chance something unhandled may occur. But dont let general-purpose error handling be an excuse for bad programming. Writing good, clean, solid code requires a lot of work and lots of "If" logic to test for things manually first, as Derek suggests. Its much easier to test for an error-like condition and alert the user (or take appropriate action) than it is to do something generic in an error handler.

For instance, something as simple as running a query and getting out a value is a lot more than checking the SqlException in case theres a Database error (fairly rare, in my experiences, though timeouts could be common in a dev environment). For instance, after excecuting DataAdapter.Fill, you should check the Rows.Count property of the table to make sure you got back a row before referencing Rows[0]. Then you need to check the column for null before blindly converting to a string. You may be thinking, "this is an insert, if it doesnt throw an SQL Exception, then it will work" but that might not be true. Who knows what might cause the insert to not return a value. Maybe a DBA changes the default "No Count" option on the database and youre suddenly getting two recordsets back. Not handling Exception means youll get a nasty default MessageBox and the dreaded NullReferenceException.

Id vote for always trapping potential errors with specific types and also always trap for the Exception. I would use try/catch wherever theres a remote chance your code might error out. If you have a simple routine, such as:
C#: 
// private int counter = 0; declared at the top of the form
private void button1_Click(object sender, System.EventArgs e)
{
    counter++;
}

I would not put any error handling on that. Granted, if the user presses the button over 2.5 billion times, you might get an overflow. But Ill take that chance :)

-Nerseus
 
I like your database example, Nerseus. Explains my point rather well.

Id pretty much always trap for Exception if theres even a remote chance something unhandled may occur.
Click to expand...
I cant agree with this logic, however. (see below for argument)

The user must be allowed to continue working
Click to expand...
I cant agree with this either. Take the AutoRecover feature in Microsoft Word. This is an excellent example of an application working correctly to prevent large amounts of data loss after an unrecoverable error has occured. For example, the exception thrown by Word caused the document buffer to be overwritten. If the exception was "handled" (I use this term loosely) the user would continue working, adding to the overwritten buffer. The buffer would contain invalid data however, and once the user saves the document he/she has lost quite a bit unknowingly. Had the application crashed, AutoRecover would have kicked in the next time Word was reopened, allowing the user to continue where they had left off (or close to it). It boils down to either taking a huge risk, putting a whole document on the line, or a small annoyance, restarting the application. Obviously the latter is a much better option.
 
First of all, a handled exception would of course not go unnoticed. Without doubt the user will be alerted, the system administrator will be alerted and the circumstances will be logged. Then the application will "rollback" to a consistent state. (It is not the case that every error is unrecoverable)

More often than not the tight jacket of user requirements, time and budget does not allow for the most comfortable solution, but one that is safe and practicable.
 
Come to think about it, I must admit that I may have a limited perspective. I have never developed an application that is designed for "standalone" usage, say a product that is targeted at private households to run on in an "unmanaged" environment.
I havent had the need to develop a strategy on error handling for such an environment.

I keep up my recommendation, however, for applications that run in "managed" environments, like larger companies with the appropriate support team, and with users that are much less likely to accept crashes than errors messages and support calls.
 
My approach is this:

If an error occurs it is displayed in a form to the user detailing what has happened. The user has a choice of clicking OK to continue using the application, rolled back so they can carry on working with some other part of the system. If it is a critical or persistent error they have the option to send an alert email to the administrator. This email details who sent it, what happened and where and asks the administrator to contact that user as soon as they can.
 
Back
Top