3

I am looking at code written by someone else.

Some of the big functions are a mess, with changes to the error handling at many points throughout the function. There is a variety of On Error Goto ErrHandler, On Error Goto 0, On Error Resume Next, as you go through the function.

What would be the best way to go through and clean up this error handling so that there is just one On Error Goto ErrHandler at the top of the function as it should be?

CJ7
  • 22,579
  • 65
  • 193
  • 321

2 Answers2

6

I don't think that having a single Error Handler is necessarily the best solution.

If you have a huge function with one error handler and something goes wrong, you have no idea at which point it failed.

It sounds to be as though the functions you describe have the correct error handling in them but that doesn't mean it is easy to read.

Maybe the best solution is to re-factor the big functions into several smaller ones which can have their own single error handler that way when an error occurs you only have a few lines to track down the bug?

You could use the existing error handler statement to give you suitable markers for code re-factoring.

Matt Wilko
  • 26,994
  • 10
  • 93
  • 143
  • I don't think it's correct to have a mixture of `On Error Goto 0`, `On Error Resume Next` and `On Error Goto ErrHandler` in the one function. – CJ7 Feb 13 '12 at 12:02
  • 2
    +1 for the refactoring part. If a piece of code needs its own errorhandling that it is likely to be a seprate piece of logic. – Dabblernl Feb 13 '12 at 12:28
  • 1
    +1. It can be correct (even essential) to have a mixture of `On Error Goto 0`, `On Error Resume Next` and `On Error Goto ErrHandler`. Especially if you are switching off error handling when executing in the VB6 IDE, using something like the link CraigJ posted on [this question](http://stackoverflow.com/questions/9206930/mouse-wheel-support-for-grid-in-vb6-app). But this can become confusing in a long function. Refactoring into smaller routines is a good approach when any large routine becomes too complex and unwieldy – MarkJ Feb 13 '12 at 13:10
  • @MarkJ: I don't agree that it is ever good to have a mixture of error handling in a function, except in the rarest of cases. The IDE example is an extremely unusual case and should be isolated to a single function. I can't see any reason not to simply have a single error handler for a function and to handle errors appropriately. Every time I have ever seen error handlers turned off I have always been able to rewrite to handle the errors properly. – CJ7 Feb 13 '12 at 13:32
  • @CraigJ I think we need to see some code, otherwise the discussion is too abstract – MarkJ Feb 13 '12 at 13:37
  • I would have voted this up *except for* the part about "refactoring." This might be advisable for other reasons but simply handling multiple exceptions in a procedure is not one of them. What some people like to call "refactoring" is often closer to: "I don't care for the way this code looks, so I'm going to waste my employer's time and money rewriting it all." – Bob77 Feb 13 '12 at 18:38
3

It's hard to say without looking at the code in question, but there are times when several different types of error handling in a single function is legitimate. In general, a single On Error GoTo ErrHandler at the top of the function serves the same function as an enclosing try/catch block in a language that supports exceptions -- it's there to catch the unexpected errors that occur.

However, within the function, there may be places where an error is a normal, expected occurrence that you want to check for inline. For example, say you're going access a network drive, where you have to account for the remote server being offline, the network down, etc. In this case, even though there's an On Error GoTo ErrHandler statement at the top, you can turn that off locally with an On Error Resume Next and try to access the drive. If an error occurs, VB6 will now just fall through to the next statement where you can check Err.Number for anything other than 0. This is analogous to a nested try/catch to trap an expected error condition that you can handle locally. Once execution has passed beyond the risky code, you have to mark the end of the section where you're going to "manually" check for errors this way with another On Error GoTo ErrHandler statement to re-enable the function-level handler.

So there's a case where more than one On Error... statment is valid. The argument can be made that the risky function should be refactored out and have its own handler, but you'd still have to deal with the specific (expected) error return values from that separate function, because the called function probably won't know what to do in the face of these errors in the context of the calling function. You would still handle that with the short-term On Error Resume Next technique.

On Error GoTo 0 disables all error handling, letting VB6 bail out of your function whenever an unexpected error occurs. I'm having a hard time coming up with a non-contrived situation where that would be a good option.

JeffK
  • 3,019
  • 2
  • 26
  • 29
  • I agree with everything except your last section. I'd prefer `On Error Goto 0` over accidentally leaving `On Error Resume Next` enabled when it shouldn't be. More damage can be done by silently ignoring errors. An error you can see is an error you can fix :) – tcarvin Feb 14 '12 at 13:20
  • 1
    Use `On Error GoTo 0` just before `Err.Raise ...` for raising custom errors from a method. – wqw Feb 14 '12 at 22:24
  • @tcarvin True. When I (used to) write VB6, I'd write the `On Error Resume Next`, then `On Error Goto Handler`, and *then* go back and fill in the code between to avoid that problem. – JeffK Feb 14 '12 at 22:59
  • @wqw That interesting. I never thought to do that because my standard handler would perform the Err.Raise, adding the function name to the Err.Description to create a pseudo call stack in the error description. e.g.: `Err.Raise Err.Number, "ProcName: " & Err.Description` When applied to nested calls, you get a result of "FunctionA: FunctionB: FunctionC: Divide by 0" as the final Err.Description. – JeffK Feb 14 '12 at 23:05
  • @JeffK: Mine too, but what information do you get from having original function in call-stack? You already know where you throw your internal error from. Skipping err handler makes the method (and class) usable outside original project (usually err handlers are project by project oriented) – wqw Feb 15 '12 at 12:19
  • @wqw: I think we're talking Err.Raise vs. unexpected errors, but I still treat both the same. Err.Raise hits the function-level handler, which adds the function name to the call stack, in front of my custom Err.Description. If you don't include the function where the error occurred, then you can't narrow down to where the error occurred. Using my example above (abbrev'd), if FuncB called FuncC and FuncD, not including "FuncC" in the call stack means all code in FuncC and FuncD is suspect. See my answer to [this](http://stackoverflow.com/questions/127645) question for a code example. – JeffK Feb 20 '12 at 20:16
  • @JeffK: Yes, you are right this might be useful. What I meant was the case when error handling is logging errors in log files. Raising an internal error surely does not need to be logged in *raising* function only to be suppressed altogether with `On Error Resume Next` in caller. If error should get logged it could be in *caller* function error handler. – wqw Feb 20 '12 at 22:08