CodeSOD: Code Commenter and Error Handler

Visual Basic for Applications represents the core mistake of putting a full-featured programming environment on every desktop. That so much VBA code is bad is not remarkable- that any good code exists would be shocking.

We rarely cover VBA code, because most of it is written by a non-programmer who discovered they could solve real business problems in Microsoft Access. TRWTF is, in fact, how much of the world runs on an Access database stuffed into a network share somewhere. But there are organizations that hire developers and then shove them into writing VBA, which is what happened to Doug. This code comes from quite awhile ago.

Doug inherited a bunch of VBA code written by another developer. It has... issues. One issue, which Doug didn't send us, was a 500 switch statement followed by a 700 line switch statement where the vast majority of the clauses were duplicated between the two, and also all the bodies were simply setting a boolean variable to true or false.

That's ugly and awful, but what Doug sent us is just weird:

Private Sub Report_NoData(Cancel As Integer) If Me.OpenArgs = True Then Else NoDataMessage End If Cancel = TrueEnd Sub

This is a weird little block of code. The empty if clause, for example, is weird. Why? Why is Cancel being set to true all the time? Why was it specified as an Integer if that's not what it holds? What exactly is NoDataMessage doing? Let's look at that.

Public Function NoDataMessage()' Code Header inserted by VBA Code Commenter and Error Handler Add-In'=============================================================' modGeneral.NoDataMessage'-------------------------------------------------------------' Purpose' Author : Joebob Bobsen, Tuesday, November 05, 2002' Notes :'-------------------------------------------------------------' Parameters'-----------''-------------------------------------------------------------' Returns:'-------------------------------------------------------------' Revision History'-------------------------------------------------------------' Tuesday, November 05, 2002:'=============================================================' End Code Header blockOn Error GoTo HandleErr MsgBox "No data was returned by this report", vbInformation, GetOption("Application Name")ExitHere: Exit Function' Error handling block added by VBA Code Commenter and Error Handler Add-In. DO NOT EDIT this block of code.' Automatic error handler last updated at Tuesday, November 05, 2002 2:12:06 PMHandleErr: Select Case Err.Number Case Else MsgBox "Error " & Err.Number & ": " & Err.Description, vbCritical, "modGeneral.NoDataMessage" End Select Resume ExitHere' End Error handling block.End Function

The core of this method is just to pop up a message box: "No data was returned by this report". Nothing about popping up the message box could cause an error, and while GetOption might if you try and access an invalid option in Access (the docs are vague on this), Application Name is a valid option, so that line of code couldn't cause an error.

It doesn't matter, though, because we handle the error anyway, thanks to an auto-generated error handler. Which I know we shouldn't get too picky about auto-generated code, but the whole thing gets wrapped up in a switch statement (select, in VBA parlance) to only have the default case.

And while the VB-style error handling of using gotos for flow control is terrible, this method adds an extra layer by bouncing back up to a label for the sole purpose of exiting the function. Which, speaking of weirdness: why is this a function in the first place? It doesn't return a value. Report_NoData should be a function, since it generates an output (through a reference parameter), but NoDataMessage is not a function, it does not return a value.

This code is confusing, perplexing, and sadly, was absolutely vital to a business process for a number of years. Clearly, someone knew the code was bad and wanted to fix it, and thus brought in code quality tools, like the "VBA Code Commenter and Error Handler Add-In", but just adding exception handling isn't enough to make the code any better, especially if the exception handling is in the wrong place.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.