4

I have a function (a event handler in a form) structured as following:

Dim errMsg as String = ""

CheckIfValidUser(..., errMsg)
If errMsg.Length > 0 Then
    ShowError(errMsg)
    LogError(errMsg)
    Return
End If

CheckIfBookAvailable(..., errMsg)
If errMsg.Length > 0 Then
   ShowError(errMsg)
   LogError(errMsg)
   Return
End If

ReserveBook(..., errMsg)
If errMsg.Length > 0 Then
   ShowError(errMsg)
   LogError(errMsg)
   Return
End If

BookReserved = True

I've noticed that most of code is in simular structure so I tried to refactor as following:

Dim errMsg as String = ""
Dim HandleError = Sub()
                      If errMsg.Length > 0 Then
                          ShowError(errMsg)
                          LogError(errMsg)
                          Return
                      End If
                  End Sub

CheckIfValidUser(..., errMsg)
HandleError() 

CheckIfBookAvailable(..., errMsg)
HandleError()

ReserveBook(..., errMsg)
HandleError()

BookReserved = True

But it won't work because I need to "return twice" instead of just returning from the nested function! Using goto doesn't work either because the exiting label is out of scope for the nested function.

Is there anyway to do this in .net? I know it is possible to return a boolean from HandleError and branch on that but then it goes back to the same repeated structure again.

Cœur
  • 37,241
  • 25
  • 195
  • 267
jack3694078
  • 993
  • 1
  • 9
  • 20
  • 8
    That's a very old style of coding. A more modern style would be to have each of the functions that performs validation to throw an *exception* if there's an issue and then (usually as high up as possible) have a single exception handler that catches these exceptions, shows messages and logs them. – Damien_The_Unbeliever Mar 06 '15 at 08:14
  • @Damien_The_Unbeliever Oh I havn't notice that it was a old style; I just find the flow is obvious when written this way. As throwing Exceptions feel somewhat ambiguous to me: You can't tell which function will / will not throw exceptions (and what exceptions) by just looking at the calls! – jack3694078 Mar 06 '15 at 08:23
  • 1
    In just the same way, you can't tell just by looking at this code what circumstances will cause `errMsg` to have a non-zero length from any of those methods. In general, good documentation goes a long way towards curing either of those ills. And, at least with exceptions, you can't *forget* to check `errMsg`. – Damien_The_Unbeliever Mar 06 '15 at 08:49

2 Answers2

7

If you modify your methods to throw an exception instead of returning a ByRef error message, your code can be rewritten as follows:

Try
    CheckIfValidUser(...)
    CheckIfBookAvailable(...)
    ReserveBook(...)
    BookReserved = True

Catch ex As Exception
   ShowError(ex.Message)
   LogError(ex.Message)
End Try

Usually, catching all exceptions ("Pokemon Exception handling") is considered bad practice, except when doing it at the outermost layer (i.e., the user interface). Since you show an error message (and since you mentioned that this is an event handler in a form), this seems to be the case here.


Another option, if you cannot change the structure of your methods, would be the following:

Dim errMsg as String = ""

CheckIfValidUser(..., errMsg)
If errMsg = "" Then CheckIfBookAvailable(..., errMsg)
If errMsg = "" Then ReserveBook(..., errMsg)

If errMsg <> "" Then
   ShowError(errMsg)
   LogError(errMsg)
   Return
End If

BookReserved = True

Personally, I'd prefer the first option, since that's the more idiomatic to the way things are done in .NET nowadays. Not having to cluster your business logic with technical error handling details is exactly what makes exceptions so powerful.

Heinzi
  • 167,459
  • 57
  • 363
  • 519
  • On second thought it actually looks pretty neat. But what if some of the methods need to release resources on fails? (e.g. Logout the book checking system if CheckIfBookAvailable fails) Nested try/catch? – jack3694078 Mar 06 '15 at 09:22
  • @jack3694078: That's what `Try...Finally` is for (since you usually want to *always* logout the book checking system, after success as well as after failure) . – Heinzi Mar 06 '15 at 09:26
  • Actually that is not the case. After reserving the book the user is required to input further information (Address, etc) before he/she can OrderReservedBook() and logout. Which is in another function (another form in fact). – jack3694078 Mar 06 '15 at 09:43
  • @jack3694078: I see, that makes sense. In that case, I'd still use an inner `Try...Finally` block, but do something like `If Not BookReserved Then Logout` in the Finally block. – Heinzi Mar 06 '15 at 10:02
2

An option to dealing with exceptions can be to rewrite your Check subs into functions returning boolean (true on success, false if errMsg contains error) and have a cascade of IF expressions each calling next function. And have a single piece of error handling code in the end

If CheckIfValidUser(..., errMsg) Then
    if CheckIfBookAvailable(..., errMsg) then
        ReserveBook(..., errMsg)
    End If
End If

If errMsg.Length > 0 Then
   ShowError(errMsg)
   LogError(errMsg)
   Return
End If

BookReserved = True

But I agree it is an old days C style...

Kirill Slatin
  • 6,085
  • 3
  • 18
  • 38