the merits of writing readable code

I inherited an Access application with 400 forms and 130 reports and I can't even guess how many lines of code. Much of that code looks like this:

Private Sub Command2_Click()
Dim x As String
x = MsgBox("You are about to generate a payroll run.  You may do this as many times as you wish before Posting Payroll.  Do you wish to proceed?", vbYesNo)
If x = 7 Then Exit Sub
DoCmd.Hourglass True
CurrentProject.Connection.Execute ("dbo.sp_GeneratePayroll")
DoCmd.Hourglass False
x = MsgBox("Generation Finished- would you like to preview the payroll report?", vbYesNo)
If x = 7 Then Exit Sub
DoCmd.OpenReport "prGenDetail", acViewPreview
End Sub

The first issue is the control name. What on Earth is Command2? Looking at the design of the form I see that it's a button with a caption of "Generate Payroll". When you add controls to an Access (or VB) form it gives them default names. The first button on a form will be Command1, the next Command2, etc. It would be like Domino Designer leaving all your fields named Untitled and just incrementing them as you add more. Most developers rename these to something meaningful, for what I hope are obvious reasons. In this application very few controls have specific names, making maintenance much more difficult.

The next issue is the utter lack of formatting. This style of coding is used throughout the application, with everything lined up at the left margin. In some rare instances there is a one or two space indentation. Lines of literal text are left so long they run off the screen. Next up is the variable, whose name doesn't tell you anything. Upon inspection you also see it is the wrong datatype. VB will handle the automatic type conversion, but why should it?

Using magic numbers instead of constants is a big no-no. If X = 7? What's 7? This will make Domino developers drool: VBA (and VB) have constants listed in Autocomplete. Press Ctrl+J and select from the list. Interestingly enough you will see that the previous developer did use the vbYesNo constant in one place, but the values everywhere else.

Finally, the application flow is a little convoluted. If...Then statements are most often done in a single line rather than in an If...Then...End If block. Why are there two Exit Sub calls? Breaking it out into If...Then...End If blocks would provide better logical flow and make it more obvious what is going on.

Here is how this routine looks now, after I addressed all those issues.

Private Sub GeneratePayroll_Click()
    Dim response As Integer
    
    response = MsgBox("You are about to generate a payroll run.  You may do this as many times " & _
        "as you wish before Posting Payroll.  Do you wish to proceed?", vbYesNo)
        
    If response = vbYes Then
        DoCmd.Hourglass True
        CurrentProject.Connection.Execute ("dbo.sp_GeneratePayroll")
        DoCmd.Hourglass False
        
        response = MsgBox("Generation Finished- would you like to preview the payroll report?", vbYesNo)
        If response = vbYes Then
            DoCmd.OpenReport "prGenDetail", acViewPreview
        End If
    End If
End Sub

This VBA code was converted to HTML using the ls2html routine,
provided by Julian Robichaux at nsftools.com.

The object name now makes sense, the code is laid out in a logical manner, the variable name tells you what it does and is of the correct datatype, long lines of literal text are broken to fit on the screen better, and the magic numbers are all gone.

One down, eleventy billion to go.