views:

230

answers:

5

Here's a question where my real world programming inexperience shines through. I have a function which makes a call to three other functions:

Public Sub StartService()
    RunSearch()
    SaveMessages()
    DeleteMessages()
End Sub

within each of the methods RunSearch(), SaveMessages() and DeleteMessages() I am using Try Catch statements to catch errors. Right now I catch the error and write to an error log when RunSearch() errors out, but I'm also get two errors from SaveMessages() and DeleteMessages() because these functions are dependent upon RunSearch() not returning an error. I am trying to build good a error catching foundation so I don't just want to kill the app when there's an error. My question is this: How can I gracefully stop execution if an error occurs in RunSearch().

+2  A: 

One possible option would be to change the RunSearch method to return a Boolean - True if successful, and False if an error occurred that would block the other two functions.

Then you could do this:

Public Sub StartService()
    If RunSearch() Then
        SaveMessages()
        DeleteMessages()
    End If
End Sub
David Stratton
+4  A: 

Why does RunSearch not rethrow the exception after logging the problem?

If you don't want to call SaveMessages() if RunSearch() fails then don't code it that way :-)

My general thought is that each method's interface is specifying a "protocol". It must state its behaviour in the event of problems. Approaches include:

  1. If I get an error I will terminate the process. This is pretty extreme, limits the reusability of your method.
  2. I will return a count or status code or somesuch. It's up to you to decide whether to check that and whether any particular status code means it's safe to call some other method. I prefer not to rely on the client to remember to check status codes.
  3. If I fail I will leave things so that subsequent processing will be sane. For example, if my job is to create a linked list then in the event of errors I will not leave a dangling pointer or an initialized list. You may get an empty list, but at least well formed subsequent processing will work. This tends to imply some degree of agreement (ie. coupling) with the other methods. This is often the nicest approach, especially when coupled with good logging of the problems.
  4. I will throw an exception if I couldn't do the work. The exception will indicate whether there's some chance of things working later if you call be again (I use TransientException and InvalidRequestException quite a lot). Exceptions are useful because the client cannot (for Java checked exceptions) accidentally ignore them. When dealing with problems such as "could not open database" this seems reasonable. We really do not want folks mistaking "couldn't even get to the database" with "this person has no criminal record".
djna
Another good suggestion. Thumbs up and an upvote for you and @Rob Thijssen.
David Stratton
Ahh. This answer is all about DESIGN. it is so tempting to dive into writing the code, but it sure helps to have a set of things to think about (and document the answers to) first.
Bill
+1  A: 

You could rethrow the method exceptions after you've done your logging, then wrap your RunSearch() method inside a try/catch block so that the next two methods don't run.

grenade
+1  A: 

It seems like you might not need low-level method-level error handlers, but only an high/application-level handler. If all your catch blocks for each method are the same, I'd suggest switching to a high-level error handler. As a general rule, you only want to catch Exceptions that you can explicitly handle, or catch them at the last oppurtunity for logging purposes, and to prevent untidy application crashses.

This is how I would approach it

public void StartService()
{
    try
    {
       RunSearch();
       SaveMessages();
       DeleteMessages();
    }
    catch(Exception ex)
    {
        //Do Nothing or Log ex
    }
}

public void RunSearch()
{
//No error handler here, as this method cannot recover from exceptions

//RunSearch functionality
}
MattH
if you indent your code 4 spaces, we get syntax highlighting...
grenade
No offense matt, but if the question is in VB from an admitted beginner, I recommend answering in VB.
Bill
@Rob. Thanks, forgot to format it@Bill. Point understood, but OP tagged the question C# so assumed it was OK
MattH
A: 

You should not use exceptions to control program flow. Use return values instead. Throwing and catching exceptions is expensive and should only be used to report truly unexpected conditions in your code.

David Stratton's answer above is a much better solution.

ScottTx