Wednesday, December 27, 2006

Why idiots shouldn't code business-critical apps in VB

Ok, so it may sound like a harsh comment, but there have been many times when I've found rather dodgy Visual Basic code in business critical applications. Unfortunately, the ease of programming with VB means that users do not have to have any knowledge of good programming practice, or even a good knowledge of VB to be able to create the next wizz bang application. Show these to your boss, and mention the fact that it took you less than a day to develop and you'll get a promotion in no time!

So why on earth do we have programmers?

To those who don't know me too well, you might think that I'm bagging Visual Basic as a language, but this is quite the opposite. The language is brilliant for RAD (Rapid Application Development) due mainly to its readability and (relatively) concise code. I use VB pretty much every day in my current workplace, and I have a library of VB code that does some pretty nifty stuff.

With Microsoft including a full Visual Basic for Applications development environment in their Microsoft Office suite, they have made it easy for users to play with. This is a good thing. However, users have a natural tendency to assume a great knowledge of programming simply because they know what a macro is.

This is even true for other programmers with little or no VB experience. All too often, companies use C/C++ developers to develop VB applications. This is fine if the developers have a genuine interest in VB and want to learn it as a language, but all to often they try to apply C/C++ concepts where they shouldn't or needn't be applied.

I put to you this block of sample VBA code that I found today in a Microsoft Access application at work. Its task is simple: to display a File Open dialog, prompting the user to select a file to open. In this case, the user wasn't a complete idiot, as it appears that they had some familiarity with the Windows API, although this could well have been an Experts Exchange copy-and-paste.

Exhibit A:


Option Compare Database 'Use database order for string comparisons
Option Explicit
Option Base 0

Declare Function upg_lstrcpy Lib "Kernel32" Alias "lstrcpy" _
(ByVal lpDestString As Any, ByVal lpSourceString As Any) As Long

Type upg_OPENFILENAME
lStructSize As Long
hwndOwner As Integer
hInstance As Integer
lpstrFilter As Long
lpstrCustomFilter As Long
nMaxCustFilter As Long
NFilterIndex As Long
lpstrFile As Long
nMaxFile As Long
lpstrFileTitle As Long
nMaxFileTitle As Long
lpstrInitialDir As Long
lpstrTitle As Long
Flags As Long
nFileOffset As Integer
nFileExtension As Integer
lpstrDefExt As Long
lCustData As Long
lpfnHook As Long
lpTemplateName As Long
End Type

Declare Function upg_GetOpenFileName Lib "COMMDLG32.DLL" _
Alias "GetOpenFileName" (OPENFILENAME As upg_OPENFILENAME) As Integer

Global Const upg_OFN_READONLY1 = &H1
Global Const upg_OFN_OVERWRITEPROMPT1 = &H2
Global Const upg_OFN_HIDEREADONLY1 = &H4
Global Const upg_OFN_NOCHANGEDIR1 = &H8
Global Const upg_OFN_SHOWHELP1 = &H10
Global Const upg_OFN_ENABLEHOOK1 = &H20
Global Const upg_OFN_ENABLETEMPLATE1 = &H40
Global Const upg_OFN_ENABLETEMPLATEHANDLE1 = &H80
Global Const upg_OFN_NOVALIDATE1 = &H100
Global Const upg_OFN_ALLOWMULTISELECT1 = &H200
Global Const upg_OFN_EXTENSIONDIFFERENT1 = &H400
Global Const upg_OFN_PATHMUSTEXIST1 = &H800
Global Const upg_OFN_FILEMUSTEXIST1 = &H1000
Global Const upg_OFN_CREATEPROMPT1 = &H2000
Global Const upg_OFN_SHAREAWARE1 = &H4000
Global Const upg_OFN_NOREADONLYRETURN1 = &H8000
Global Const upg_OFN_NOTESTFILECREATE1 = &H10000
Global Const upg_OFN_SHAREFALLTHROUGH1 = 2
Global Const upg_OFN_SHARENOWARN1 = 1
Global Const upg_OFN_SHAREWARN1 = 0

'SNIPPED MANY LINES OF CODE

' Developed by: (NAME REMOVED TO PROTECT THE GUILTY), Inc.

'SNIPPED MANY MORE LINES OF CODE


Function upg_GetDBFileName$(stype As String, sOpenPrompt$)
Dim szFilter As String
Dim Retval As Variant
Dim wFlags As Integer
Dim varFileName As Variant
Dim szDirectory As String
Dim sPrompt$

' Specify that the chosen file must already exist if stype <>NEW

If stype = "new" Then
wFlags = 0
sPrompt = "New Database"
Else
wFlags = upg_OFN_FILEMUSTEXIST1 Or upg_OFN_HIDEREADONLY1
sPrompt = sOpenPrompt
End If

szDirectory = "C:\"


'* Define the filter string and allocate space in the "c" string
'* Duplicate this line with changes as necessary for more file templates.
szFilter = szFilter & "Access Database (*.mdb;*.mdd)" & Chr$(0) & "*.mdb;*.mdd" & Chr$(0)


' Now actually call to get the file name.
varFileName = upg_OpenCommDlg(szDirectory, szFilter, wFlags, "MDD", sPrompt)
If Not IsNull(varFileName) Then
' strip the trailing nulls
varFileName = Left(varFileName, InStr(1, varFileName, Chr(0)) - 1)
Else
varFileName = ""
End If
upg_GetDBFileName = varFileName
End Function

Function upg_OpenCommDlg( _
szInitialDir As String, szFilter As String, wFlags As Integer, _
szDefExt As String, szTitle As String) As Variant
Dim OPENFILENAME As upg_OPENFILENAME
Dim szFileName As String
Dim szFileTitle As String
Dim szTitleStr As String
Dim szCurDir As String
Dim wAPIResults As Integer
Dim CRLF$

CRLF$ = Chr$(13) & Chr$(10)

'* Allocate string space for the returned strings.
szFileName = Chr$(0) & Space$(255) & Chr$(0)
szFileTitle = Chr$(0) & Space$(255) & Chr$(0)

'* Give the dialog a caption title.
szTitleStr = szTitle & Chr$(0)

'* Set up the defualt directory
szCurDir = szInitialDir & Chr$(0)

'* Set up the data structure before you call the GetFileNameName

OPENFILENAME.lStructSize = Len(OPENFILENAME)
' This ought to be a legal hWnd, but maybe there's no form open. So we'll
' use 0 here. Windows doesn't seem to mind.
OPENFILENAME.hwndOwner = 0
OPENFILENAME.lpstrFilter = upg_lstrcpy(szFilter, szFilter)
OPENFILENAME.NFilterIndex = 1
OPENFILENAME.lpstrFile = upg_lstrcpy(szFileName, szFileName)
OPENFILENAME.nMaxFile = Len(szFileName)
OPENFILENAME.lpstrFileTitle = upg_lstrcpy(szFileTitle, szFileTitle)
OPENFILENAME.nMaxFileTitle = Len(szFileTitle)
OPENFILENAME.lpstrTitle = upg_lstrcpy(szTitleStr, szTitleStr)
OPENFILENAME.Flags = wFlags
OPENFILENAME.lpstrDefExt = upg_lstrcpy(szDefExt, szDefExt)
OPENFILENAME.hInstance = 0
OPENFILENAME.lpstrCustomFilter = 0
OPENFILENAME.nMaxCustFilter = 0
OPENFILENAME.lpstrInitialDir = upg_lstrcpy(szCurDir, szCurDir)
OPENFILENAME.nFileOffset = 0
OPENFILENAME.nFileExtension = 0
OPENFILENAME.lCustData = 0
OPENFILENAME.lpfnHook = 0
OPENFILENAME.lpTemplateName = 0

'* This will pass the desired data structure to the Windows API,
'* which will in turn it uses to display the Open Dialog form.

wAPIResults = upg_GetOpenFileName(OPENFILENAME)
upg_OpenCommDlg = IIf(wAPIResults = 0, Null, szFileName)
End Function


 


WTFs in order:


  1. What do you plan to achieve with the lpstrcpy function?
    lpstrcpy is a kernel function to copy a string from one location to another. It happens to return a pointer to the destination string when successful. These idiots are using a string copy function to return a handle to a string. In VB. Idiots.
  2. Why are the lpstr's in upg_OPENFILENAME declared as long integers?
    Ok, so an lpstr is a long integer pointer to a string. So a C programmer would declare it as a long. But strings in VB are stored with pointers, just like they are in every other language I know. So you can declare lpstr's as Strings and save a lot of time and effort.
    The only noticable difference is that VB strings are not null terminated, while most Windows API functions expect them to be. This is easily resolved by appending the ASCII character &H0 (stored in the VB constant vbNullChar) to the end of the string before calling the API function.
  3. Who on earth still uses dollar signs, percentages and ampersands to declare the data types of BASIC variables?
    Last time I used things like "name$" and "id%" was when I was coding in GW-BASIC and QuickBASIC. Yes, in MS-DOS. Correct me if I'm wrong, but since at least version 4 of Visual Basic (ie: pre-VBA in MS Office) you have been able to use the As keyword to declare the data type of a variable. So "Dim name$" becomes "Dim name As String" and "id%" becomes "Dim id As Integer". Much more readable, and the added bonus is if you decide to change the data type of an existing variable, you don't need to find and replace all existing instances of the variable name. Sweet, huh?
  4. What's with the stype parameter to upg_GetDBFileName$?
    Why use a string to represent a parameter that can only have two values: new or old? That's what a boolean is for. Declare the parameter as "bNewFile As Boolean" or similar. Or drop the parameter completely and display "New Database" if the sOpenPrompt$ parameter is blank.
  5. What's with the variants?
    In Visual Basic, simple data types are not derived from the object data type. This is because BASIC was never designed as an object-oriented language. Visual Basic has the variant data type as hack to allow programmers to store either simple data types or an object in the same variable. For instance, a variant can contain a string, an integer or a recordset. This is similar to declaring a variable as as object in languages such as Java. Like the object type in Java, you should avoid it's use at all costs. If you know what the data type of a variable will be at development time, there is no excuse to use variants. Variants = BAD.
    In this case, variants have been used to allow the upg_OpenCommDlg function to return Null. While it may be common for C functions to return null, this is not common practice in VB. This is because the Null data type is used (particularly when dealing with databases) to represent an unknown value. In this case, if the call to the Windows API returns zero, (ie: the dialog box was cancelled or closed) then the filename is empty, but it is not unknown. The function should return "" (an empty string) or to be more correct, the VB value Empty. This would allow the function to be defined as returning only strings, not variants. (For more information on the difference between Null and Empty in VB see Eric Lippert's post on MSDN.)
    What makes this all the more entertaining is that the function upg_GetDBFileName$, (which is the only function in the application to call upg_OpenCommDlg,) checks to see if the value is Null and, if so, returns an empty string. Why not do this in upg_OpenCommDlg and save the effort of an extra IF statement?
  6. Why specify the initial directory to be C:\?
    Never assume that C:\ exists. The user may have their CD drive as C:\, or they may not have access to view C:\ due to security settings.
    And if you don't specify a folder when calling methods of the Common Dialog API, it will default to the user's home directory, which is likely to be the location of any document they are about to open anyway.
  7. Why declare CRLF$?
    This is pointless for the following reasons:
    1. vbCrLf is a Visual Basic constant that would replace it.
    2. They don't use it in this code any way.
  8. What's with the Chr$(0)'s?
    How about using vbNullChar and save yourself a function call or two. The two methods are equivalent, so this is only a minor WTF, but constants are always a good idea.
  9. What's with the null characters at the end of the null strings?
    The programmers allocate szFileName as empty space to store a string using the common method of Space(255) to return a string of 255 space characters. In this instance, they've decided to improve on that by making it 257 characters with a null at each end. Why? Who knows. The API function will see that the first character is null, and consider it to be a zero length string. Upon the function's return, the memory allocated by the Space(255) function may be overwritten if the API decides to change the value of this variable. So the end null character is pointless.
    Note that the Space function in VB is not a memory function that allocates space of 255 bytes, it is a string function that returns a string containing the specified number of ASCII space characters. The actual character is irrelevant when calling the API, as it will get overwritten anyway.
  10. What's going on with hwndOwner?
    The Common Dialog API in Windows provides standard looking File Open/File Save/Print and Color Chooser dialog boxes. Because it creates these dialog windows from outside your application, hwndOwner is used to pass a handle to a window representing the owner of the dialog box. In most applications, this will be the handle of the window containing the button or menu item that triggered the dialog box to be displayed. This allows Windows to ensure that the dialog box stays modal in that application until such time as it has been closed. This also reduces the chances of having an orphaned dialog box drifting through the Z-order as you Alt-Tab between applications.
    In most VB applications, your call to a wrapper function such as upg_OpenCommDlg would contain a parameter of type Form. Every window in VB is a Form. Every Form as a property called hWnd which returns the underlying handle used by Windows as an identifier of that window. Therefore, you can provide the Common Dialog API with an hWnd to keep the relationship between the application and the dialog box.
    In this application, as noted in the comments, the developers weren't sure if a window would currently be displayed by their application, so they default the value of hWndOwner to zero, making the dialog box an orphan. Initially, their comment sounded beliveable. This function is buried deep in the code of the application, and could theoretically be called at anytime. However, this is a GUI application. All GUI applications have windows, and at least one of those windows will always be loaded. That window may not be visible, but Windows will still know it exists, so it will have an hWnd.
    To make the matter worse, because this application is a Microsoft Access database, they could have easily used the Application.hWndApplication function to return the handle to the Microsoft Access window, irrespective of what is happening in the database itself.
  11. (See WTF 1 regarding the blatant abuse of the lpstrcpy function)

While this appears to be old code that may have been written for Microsoft Access '97, I thought it would be good to show how I would do this in recent versions of Microsoft Access:



Function PromptForDBFileName(Optional sOpenPrompt As String = "") As String
Dim fd As FileDialog
Set fd = Application.FileDialog(msoFileDialogFilePicker)

fd.Title = IIf(sOpenPrompt = "", "New Database", sOpenPrompt)
fd.Filters.Add "Access Database (*.mdb;*.mdd)", "*.mdb;*.mdd"

If fd.Show Then PromptForDBFileName = fd.SelectedItems(1)
End Function

 


Note that this function replaces all the code shown above.


I'm glad I've gotten that off my chest. Now to tidy this mess up...