I've automated downloading of CSVs from our vendors' websites. As expected with anything web-based, things can go... wrong, so I'm implementing some better error handling.

The code has been working reasonably stably for a year or so, but when failures have occurred in the download phase, the other phases have attempted to process anyway which is just silly. So now the intent is that archiving the freshly downloaded file and attempting to load it to the appropriate SQL table will only happen if the file is actually freshly downloaded.

So far,I've made some updates to use Err.Raise when a failure happens at any of the many steps along the way. These are now correctly bubbling up to the main code loop which is dropping into the ErrorHandler: as expected.

### Welcome to the World of Spaghetti™:

What I don't like about how this is shaping up is that the error handler now has a bunch of Resume {label} code and it feels like I've regressed to 8th grade and am writing some Apple ][ Basic by just GoToing everywhere and even Resuming into the middle of a While loop.

I'm looking for some suggestions on how to refactor ProcessLoad to better support this method of handling things, or a better solution (that maybe doesn't involve raising errors) for tracking what the current status is and whether subsequent steps should process.

### The Dilemma:

The key to my dilemma is that each file download/archive/SQL Load is an independent process. If one fails, there's no reason to abort the whole thing. The load is set up to handle the fact that I might be getting the same CSV file multiple times and to ignore (not load to SQL, though it will attempt to archive it - that's the next thing to fix) any duplicate data in it. I also have the option in my user interface to select all the files that need to be downloaded, or pick and choose just those that I want to download this particular time.

A couple of notes:

• This is existing, functional code that's in the midst of getting a rehab, so things aren't quite as consistent as one might like.
• Yes, I'm aware that I don't handle every error status yet. I don't like how this is shaping up, so I'm looking for input prior to writing more code that has to be changed.

Option Compare Database
Option Explicit

'@Folder("WebDriver")

FailureToCreateDriver = vbObjectError + &H1&
FoundNoSatSurveyClinics = vbObjectError + &H2&
FailToNavigate = vbObjectError + &H4&
OK = 0
End Enum

companyName As String
statusMessage As String
okToContinue As Boolean
End Type

Public Property Get okToContinue() As Boolean
okToContinue = True
Else
okToContinue = False
End If
End Property

Public Property Get companyName() As String
companyName = this.companyName
End Property
Public Property Let companyName(ByVal value As String)
this.companyName = value
End Property

End Property
End Property

End Property
End Property

Public Property Get statusMessage() As String
statusMessage = this.statusMessage
End Property
Public Property Let statusMessage(ByVal value As String)
this.statusMessage = value
End Property


Public Sub ProcessLoad(ByVal FileType As DownloadDataType, ByVal clinicList As ListBox)

On Error GoTo ErrorHandler

Dim Index As Long
Dim DataSources As Scripting.Dictionary

Select Case FileType
Case SnapSurvey
Case ApptPlus
End Select

Index = 0
While Index < clinicList.ListCount
clinicList.Selected(Index) = True
Select Case FileType
Case SnapSurvey
Case ApptPlus
End Select

Dim ArchiveData As ZipToolsData
Dim Archiver As ZipTools
ArchiveSourceDataByClinicID DataSources(FileType), clinicList.Column(0, Index), ArchiveData, Archiver

'todo: Phase 4: give better result reporting so I know if there was an error loading or just no data to be loaded.
NextClinic:
clinicList.RemoveItem (Index)
Else
clinicList.Selected(Index) = False
Index = Index + 1
End If
Wend

CleanExit:
If Not Downloader Is Nothing Then 'we could get here now without ever having properly set up the downloader class, so skip as necessary
End If
End If

MassiveAbort:

Exit Sub

ErrorHandler:
With Err
LogManager.Log ErrorLevel, Strings.Join(Array("Source: ", .Source, " Error Num: ", .Number, " Description: ", .Description), vbNullString)
If .Number = FailureToCreateDriver Or .Number = LoginFailure Then
Resume MassiveAbort
Resume NextClinic
End If
End With

Resume CleanExit

End Sub


It seems I may not have included quite enough code. Here is a path through all methods necessary to initially bring up the web page (prior to logging in) including all the Err.Raise to send the messages back. (No implication that there are faults with the current review posted - I just want to be sure.)

Public Function GetSnapSurveyFile(ByVal SSWD As IWebDownloader, ByVal clinicID As Integer, ByVal clinicName As String, ByRef webDLStatus As WebDownloaderStatus) As Integer

On Error GoTo ErrorHandler

Dim SQLString As String
SQLString = "SELECT ExcelFileName " & _
"  FROM ExcelLoadConfig " & _
" WHERE ClinicID = " & clinicID & _
"   AND ExcelFileType = 'SatSurvey'"

LogManager.Log TraceLevel, SQLString
RecSet.Open Source:=SQLString, ActiveConnection:=CStr(TempVars!constring)
While Not RecSet.EOF And Not RecSet.BOF
clinicLinkText = LCase(RecSet.Fields(0))      'note: the links are case sensitive when finding them on the web site
With SSWD
.fileName = clinicLinkText & Tools.GetGeneralConfig("GetSnapSurveyFile", "FileExtension", "SatSurvey")
End With
RecSet.MoveNext
Wend

CleanExit:
RecSet.Close
End If

Exit Function

ErrorHandler:

Resume CleanExit

End Function


Note that OP is aware of SQL injection and the dangers of building the query string this way vs a parameterized query. However the parameter being built into the string comes from the database itself and is passed in with no opportunity for an end user to make a malicious change to it. There are only two of us with access to the database and neither of us wants to put in the effort to clean up after a malicious attack. Should this be changed? Yes. Probably won't happen today.

.DownloadDriver is implemented here:

Private Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)

On Error GoTo ErrorHandler
End If

If Not this.IsLoggedIn Then
End If
If this.IsLoggedIn Then
End If
End If
End If

CleanExit:
Exit Sub

ErrorHandler:
With webDLStatus
End With
Resume CleanExit

End Sub


And the page itself is loaded here:

Private Sub IWebDownloader_LoadWebPage(ByRef webDLStatus As WebDownloaderStatus)

'' NOTE: If downloads fail with err.Number = 13, it's likely that a new ChromeDriver is needed
''  Check here for an updated version:
''  Put the chromedriver.exe file here:
''     C:\Users\<user>\AppData\Local\SeleniumBasic
''  note that from <user> down are hidden directories and "Find" won't search them.

On Error GoTo ErrorHandler

Dim startTime As Single
startTime = Timer
this.IsPageLoaded = this.Driver.Get(this.URL, timeout:=60000)   'give a generous 30 seconds for the page to load
Dim EndTime As Single
EndTime = Timer
this.Driver.Timeouts.ImplicitWait = CLng((EndTime - startTime) * 2000)  'dynamically adjust the timeout to the current page load speed

CleanExit:
Exit Sub

ErrorHandler:
With Err
LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
If .Number = 13 Then
LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
End If
webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
End With
Resume CleanExit

End Sub


OP notes that this is a good example of why code comments are so dangerous - the code gives a generous 60 seconds before timing out, however the comment says only 30 seconds. I'll just remove that comment and escort the OP to the door...

While I can comment on some of the coding aspects, I don't have the actual experience in downloading to anticipate all errors or problems. So, I may leave breadcrumbs to some solutions rather than anything specific.

This is the lowest level, so any issues that bubble up from here are going to get harder to fix at the higher levels.

If I assume that On Error is a valid approach here, we can still modify the error handler in the first instance:

ErrorHandler:
With Err
LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
If .Number = 13 Then
LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
End If
webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
End With
Resume CleanExit


becomes

ErrorHandler:
With Err
LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
If .Number = 13 Then
LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
End If
webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
End With
Resume CleanExit


You will notice that the only thing I did was remove the Raise. This means that your routine will now have an orderly exit rather than an abortive one.

You set this.IsPageLoaded in the code, but do not check it before running to the next part of the code. You can avoid one error call by simply checking if this.IsPageLoaded is False. What are the other ways the .Get can error out?

Similar to the previous section:

ErrorHandler:
With webDLStatus
End With
Resume CleanExit


becomes

ErrorHandler:
Resume CleanExit


Which means, that you could optimise even more:

Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)
On Error GoTo CleanExit ' was ErrorHandler
[...]
CleanExit:
End Sub


This is a real soft fail, but you don't do anything except raise an error (an abortive rather than a controlled exit). Because of this, I assume that you are merely passing through any error from LoadWebPage.

Now you can simply check for errors in webDLStatus. Actually, you don't have to check for anything, because webDLStatus is set by the lower routines. So your new code could be:

Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)
End If

If Not this.IsLoggedIn Then
End If
If this.IsLoggedIn Then
End If
End If
End If
End Sub


This means this routine is simply a flow logic control point.

## GetSnapSurveyFile

At first glance, it seems your error handler is simply taking the passed-through error and creating a soft fail. A big issue here is that once an error is raised, you forcibly exit the loop and stop processing all the other records.

Now that we have stopped raising errors in the lower-level you can handle a download error (as reported through the download status) in a more controlled way. Perhaps If webDLStatus.DownloadStatus > 0 Then Move.EOF if you really want to abort the loop.

You also don't check for errors in the database access functions. What happens if it can't find clinicLinkKey (and where is this set anyway)? These are the sort of errors that can be anticipated. I suggest that you actively trap these errors and update webDLStatus to reflect the outcome. Of course, this is in a loop and you only have one status report going out.

As per the previous comment, because you don't do anything in your error handler except go for a clean exit, you can (if you need to keep On Error) just go to CleanExit in the first place.

• The removal of the .raise is what gets this guy the checkmark (not that it matters). That's what I've ended up doing. Thanks again for your time! – FreeMan Apr 5 '18 at 14:05

## Some basics

Trivial and not really going to affect your current code, but consider setting DownloadStatuses as flaggable errors (instead of H1,H2,H3, ... use H1, H2, H4, ...). That way you can flag multiple errors at the one time.

VBA has limitations on user-defined types (e.g. Private Type TDownloadStatus), particularly how they can be used in Collections. I always use Classes now - just as easy to code and modern machines don't sweat the extra load. Perhaps this would be a considerations if you were dealing with big data.

• As an added bonus, you can make your enum part of the class, not the main code, thus encapsulating more code.
• Having said the above, I do see that you have actually used WebDownloaderStatus class to encapsulate the UDT (for reflection?) which addresses the points in my main paragraph. Why not just run with the Class and have the elements of the Type as private members of your class? You have effectively added an extra layer of code - but for what real value?

Be really critical on yourself and review how logic code is written. Classic example that I see often:

Public Property Get okToContinue() As Boolean
okToContinue = True
Else
okToContinue = False
End If
End Property


should be:

Public Property Get okToContinue() As Boolean
End Property


Error handling (basic philosophy): If you can foresee errors, you should always explicitly handle them, rather than leaving it to exceptions (on in VBA's case, On Error/Err.Raise).

Is this complete? Set DataSources = SetDataSources, but I don't see SetDataSources defined anywhere. I assume this is a global object?

Double check your properties in the WebDownloaderStatus class - you have not defined DownLoadDataType in the code in the OP, so should this be a Let or a Set?

Dealing with the basics helps cut some of the Spaghetti. This makes it clearer to address your main question - the program flow.

## Error Handling and flow

Your Select Case FileType do not include a Case Else statement, but you also do not validate FileType on entry to the function. This is one area where an error can occur, because it means a possible path for Downloader = Nothing, which your other code does not directly address.

You can refactor your working subroutines (I assume they are Sub because you don't use any return values) to become functions, and they can return an error code. Even better, they could return a WebDownloaderStatus object, which holds all the relevant information to help your main code flow.

And then it hits me - the obvious. I assume the Get~File subroutines fill downloaderStatus because I assume downloaderStatus is passed ByRef (it is an object after all). (See comment above about returning it from a function instead) - but downloaderStatus is not used to check results.

Rather than throw errors in the subordinate routines, you can simply pass the errors back in the downloaderStatus object and let your main code decide how to handle it. I reckon that, with proper use of the downloaderStatus object you could get rid of any need for On Error.

Should Get~File return a download status, or should it return the Downloader? Could be 6 of one, half dozen of the other - but this minor refactoring will help with a logical program flow even if it does not appear to make any significant change to the outcome or efficiency.

## Important next step

Having gone through my thoughts above, your Spaghetti is not the problem itself, it is a symptom of other problems in your subordinate code.

Review your subordinate code and make it safe. Create these routines in a way that any errors are caught (or averted in the first place) and reported through the status object. After all, this is why you developed WebDownloaderStatus class.

Once you have done that, you will find that your ProcessLoad will become cleaner and maintainable.

• You know how sometimes you can't see the forest for the trees? Yeah... Thanks. I was actually probably about 85% of the way to this answer, but had been distracted from working on this by other critical things. When I got back to it, I tried over complicating things. I'll also look into the variety of other items you mentioned in both reviews, as well. – FreeMan Apr 2 '18 at 14:15