Intercepting (Try/Catch) on binding...

[mandelbrot]

Dear All,


I've written a class which obtains data from a database. When no data is obtained an exception is raised. Because of the structure of the class I've incorporated several Try/Catch blocks which feed exceptions back.

The class can be instantiated without arguments. This forces it to use a default value, however, if no data is found, even though I have a Try/Catch block surrounding the instantiation of the class, the exception is not caught...

Am I missing something?


Paul.

 

[snarfblam]

What does the try/catch look like? Are you catching all exceptions or only certain exceptions? What is the exception? Does the IDE break in the constructor code or the try/catch block?

 

[mandelbrot]

Hi Marbe,


Well the exception that's making it fall over is home rolled, simply stating that no data was found. (I could have created a flag to tell me this, but I thought that due to the topic concerned - user information - I'd implement Try Catch blocks). The blocks appear to work fine up until the Try Catch that's implemented in the constructor. Even though I've surrounded the instantiation of the class in a Try Catch block, it's not catching the Exception...

...
Try
    _User = New NTLogin
Catch ex As Exception
    'Deal with exception here...
    ...
End Try
...

And in the NTLogin class...

Public Sub New()
    Try
        Load(Environment.Username & "\\" & Environment.UserDomain)
    Catch ex As Exception '              [color=red]<------ Falls over here![/color]
        Throw ex
    End Try
End Sub
...
Public Sub Load(ByRef pUser As String)
    ...
    Else
        Throw New System.Data.DataException("User not found.")
    End If
    ...
End Sub

There is another piece to the jigsaw, but it works fine, as does the Load routine.


Paul.

 

[PlausiblyDamp]

What do you mean when you say it falls over on that line?

If you step through the code is it catching the exception? If so the only thing you are doing is throwing it again...

 

[mandelbrot]

Hi PD,


What I'm asking is: Shouldn't the Exception pass from the Catch block and be passed to the instantiation? (So that the Load passes the exception to New, which then passes it to the code performing the instantiation...)

Actually, I've got the line wrong for the 'Falling Over' bit - it should be the End of the Catch block.


Paul.

 

[PlausiblyDamp]

Exceptions will be passed up until they are caught by an appropriate excpetion handler, if there is no suitable handler then it will crash.

Also you do not need to throw ex here - if you do not handle the exception it will automatically be passed up to the 1st suitable handler.

 

[mandelbrot]

Ok, here's the actual code...

The instantiation:

        Try
            _User = New NTLogin
        Catch ex As Exception
            'Deal with no users...
            MessageBox.Show("You do not have an account on this system." & _
                coreUtils.Util.crlf(2) & _
                "Speak to the administrator to obtain registration to this system.", _
                "No such user", MessageBoxButtons.OK, MessageBoxIcon.Stop)
            Me.Close()
            End
        End Try

NTLogin constructor:

    'Overloaded constructor defines empty, loads given id or username, or creates a new user...
    Public Sub New()
        'This version will automatically laod the current user as an account...
        Try
            Load((Environment.UserDomainName & "\" & Environment.UserName).ToLower)
        Catch ex As Exception
            Throw ex
        End Try
    End Sub

NTLogin Load function:

    Public Sub Load(ByVal pUsername As String)
        Dim sqlString As String = "select * from [userUser] where [USERNAME]='" & pUsername & "' "
        Dim localOR As OpenReader = DataService.GetReader(connString, sqlString) 'New Parameters("@pUsername", pUsername)
        Try
            GetUser(localOR)
        Catch ex As Exception
            Throw ex
        End Try
        localOR.Dispose()
    End Sub

NTLogin GetUser:

    'Get the user detail from the openreader...
    Private Sub GetUser(ByVal pReader As OpenReader)
        Dim noUser As Boolean = False
        Try
            If pReader.Reader.Read Then
                _UserId = CType(pReader.Reader.Item("USERID"), Integer)
                _Username = pReader.Reader.Item("USERNAME")
                _Forename = pReader.Reader.Item("FORENAME")
                _Surname = pReader.Reader.Item("SURNAME")
                _Locked = pReader.Reader.Item("LOCKED")
            Else
                noUser = True
            End If
        Catch ex As Exception
            Throw ex
        End Try
        'Check to see if the user exists...
        If noUser Then
            Throw New Security.SecurityException("User not registered.")
        End If
    End Sub

As you can see, each try catch block is defined to handle a basic exception class (ex As Exception). In theory, everything should cascade down to the constructor and then to the calling class, but it doesn't - it stops in the handler in the constructor.

Seems odd to me!



Paul

 

[PlausiblyDamp]

Firstly there is no need to catch and throw the exception if you are doing nothing with it - just don't bother catching it and it will bubble up the exception handler chain till it is handled. Putting lots of code of the form

Try
    'do stuff
Catch ex as exception
    throw ex
end try

Is almost the same as

'do stuff

the main differences being the first case loses the stack trace and will cause additional overheads while the second one maintains the stack trace correctly and doesn't have the same associated overheads..

Secondly catching Exception is generally a bad idea as it means you are willing to deal with any and all errors regardless of what they are; often it is better to just trap the specific errors you know how to deal with. In your case te only exception you are throwing is a SecurityException so only catch that type - if other exceptions get thrown then at least you are nor sweeping problems under the carpet.

Thirdly throwing an exception from a constructor is a big no-no (either explicitly or implicitly) as it leaves the system in a potentially confused state. i.e.

object o = new <whatever>

should always leave o pointing to a valid object - if you throw an exception then this doesn't happen and therefore goes against the recomended approach.

Try removing the throw from the constructor and see if the error goes away. If so you might want to re-engineer your code to use a shared Create method on the class to instantiate instances instead of using the constructor.

 

[mskeel]

If all you are doing is rethrowing the exception such as in GetUser() and Load(), then you might as well not put everything in a try..catch block -- you aren't actually handling the exception there, you are handling it at the instantiation level. Let the exception fall all the way up to the instantiation level without touching it. If you did need to handle something, say in the GetUser() method such as closing a DB connection maybe, then you would want to keep the try..catch, take action after the exception is thrown, then rethrow. But since you aren't doing anything special you are just doing more work for yourself.

Perhaps this will make debugging a little easier?

 

[mandelbrot]

So what you're saying is that it would be easier to simply skip all of the error trapping (unless it's to trap other problems) and handle this programmatically some other way...

Well, I did actually do that this afternoon, and wrote the system to set a flag (in this case, simply setting the name of the user to '<NOUSER>') and that worked fine.

From what I can see, if there is a problem with the data, the system would fall over anyway...

Thanks guys, I'll restructure to the utilise that method and remove the error trapping.