Thomas Jefferson's Accidental Programming Advice
Premature declaration is nothing to be embarrassed about. It can happen to anyone. You don't want to make a habit of it, though.
The government closest to the people serves the people best.
This Jeffersonian principle also applies to variable declaration. Mutatis mutandis:
The variable declared closest to the code serves the code best.
This is all an unnecessarily highfalutin way to say STOP DECLARING ALL OF YOUR VARIABLES AT THE TOP OF YOUR PROCEDURES!!!!
But don't take my word for it.
Code Complete to the Rescue
Let's refer to what I consider the authoritative text on practical programming advice, Code Complete: Second Edition, by Steve McConnell.
Here's an extended excerpt covering this topic from pages 241 - 242 of the book:
10.3 Guidelines for Initializing Variables
Improper data initialization is one of the most fertile sources of error in computer programming. Developing effective techniques for avoiding initialization problems can save a lot of debugging time. The problems with improper initialization stem from a variable’s containing an initial value that you do not expect it to contain.
...
Following are guidelines for avoiding initialization problems:
...
Initialize each variable close to where it’s first used Some languages, including [classic] Visual Basic, don’t support initializing variables as they’re declared. That can lead to coding styles like the following one, in which declarations are grouped together and then initializations are grouped together—all far from the first actual use of the variables.
Example of Bad Initialization
' declare all variables
Dim accountIndex As Integer
Dim total As Double
Dim done As Boolean
' initialize all variables
accountIndex = 0
total = 0.0
done = False
...
' code using accountIndex
...
' code using total
...
' code using done
While Not done
...A better practice is to initialize variables as close as possible to where they’re first used:
Example of Good Initialization
Dim accountIndex As Integer
accountIndex = 0
' code using accountIndex
...
Dim total As Double
total = 0.0
' code using total
...
Dim done As Boolean
done = False
' code using done
While Not done
...The second example is superior to the first for several reasons. By the time execution of the first example gets to the code that uses done, done could have been modified. If that’s not the case when you first write the program, later modifications might make it so. Another problem with the first approach is that throwing all the initializations together creates the impression that all the variables are used throughout the whole routine—when in fact done is used only at the end. Finally, as the program is modified (as it will be, if only by debugging), loops might be built around the code that uses done, and done will need to be reinitialized. The code in the second example will require little modification in such a case. The code in the first example is more prone to producing an annoying initialization error.
This is an example of the Principle of Proximity: keep related actions together. The
same principle applies to keeping comments close to the code they describe, keeping loop setup code close to the loop, grouping statements in straight-line code, and to many other areas.
I'm Sorry, Microsoft, But You Are Wrong
Seemingly every VBA example Microsoft publishes reinforces this bad habit.
Sub GetProductStats()
Dim dbsNorthwind As DAO.Database
Dim rstProducts As DAO.Recordset
Dim rstCategories As DAO.Recordset
Dim varFirstMark As Variant
Dim varHighMark As Variant
Dim varLowMark As Variant
Dim curHighRev As Currency
Dim curLowRev As Currency
Dim strSQL As String
Dim strCriteria As String
Dim strMessage As String
On Error GoTo ErrorHandler
Set dbsNorthwind = CurrentDb
strSQL = "SELECT * FROM Products WHERE UnitsOnOrder >= 40 " & _
"ORDER BY CategoryID, UnitsOnOrder DESC"
Set rstProducts = dbsNorthwind.OpenRecordset(strSQL, dbOpenSnapshot)
If rstProducts.EOF Then Exit Sub
StrSQL = "SELECT CategoryID, CategoryName FROM Categories " & _
"ORDER BY CategoryID"
Set rstCategories = dbsNorthwind.OpenRecordset(strSQL, dbOpenSnapshot)
' For each category find the product generating the least revenue
' and the product generating the most revenue.
Do Until rstCategories.EOF
strCriteria = "CategoryID = " & rstCategories![CategoryID]
rstProducts.FindFirst strCriteria
curHighRev = rstProducts![UnitPrice] * rstProducts![UnitsOnOrder]
If Not rstProducts.NoMatch Then
' Set bookmarks at the first record containing the CategoryID.
varFirstMark = rstProducts.Bookmark
varHighMark = varFirstMark
varLowMark = varFirstMark
' Find the product generating the most revenue.
Do While rstProducts![CategoryID] = rstCategories![CategoryID]
If rstProducts![UnitPrice] * rstProducts![UnitsOnOrder] > _
curHighRev Then
curHighRev = rstProducts![UnitPrice] * _
rstProducts![UnitsOnOrder]
varHighMark = rstProducts.Bookmark
End If
rstProducts.MoveNext
Loop
' Move to the first record containing the CategoryID.
rstProducts.Bookmark = varFirstMark
curLowRev = rstProducts![UnitPrice] * rstProducts![UnitsOnOrder]
' Find the product generating the least revenue.
Do While rstProducts![CategoryID] = rstCategories![CategoryID]
If rstProducts![UnitPrice] * rstProducts![UnitsOnOrder] < _
curLowRev Then
curLowRev = rstProducts![UnitPrice] * _
rstProducts![UnitsOnOrder]
varLowMark = rstProducts.Bookmark
End If
rstProducts.MoveNext
Loop
End If
' Set high and low bookmarks to build the message string.
strMessage = "CATEGORY: " & rstCategories!CategoryName & _
vbCrLf & vbCrLf
rstProducts.Bookmark = varHighMark
strMessage = strMessage & "HIGH: $" & curHighRev & " " & _
rstProducts!ProductName & vbCrLf
rstProducts.Bookmark = varLowMark
strMessage = strMessage & "LOW: $" & curLowRev & " " & _
rstProducts!ProductName
MsgBox strMessage, , "Product Statistics"
rstCategories.MoveNext
Loop
rstProducts.Close
rstCategories.Close
dbsNorthwind.Close
Set rstProducts = Nothing
Set rstCategories = Nothing
Set dbsNorthwind = Nothing
Exit Sub
ErrorHandler:
MsgBox "Error #: " & Err.Number & vbCrLf & vbCrLf & Err.Description
End SubHere's what it looks like if we move the declarations to the lines just before we reference them. Also, because the variables are declared so close to where they get used, the already-atrocious Systems Hungarian prefixes (e.g., var, cur, str, etc.) truly serve no purpose now, so I removed them entirely. Here's the rewritten code in my preferred style:
Sub GetProductStats()
On Error GoTo ErrorHandler
Dim dbsNorthwind As DAO.Database
Set dbsNorthwind = CurrentDb
Dim SQL As String
SQL = "SELECT * FROM Products WHERE UnitsOnOrder >= 40 " & _
"ORDER BY CategoryID, UnitsOnOrder DESC"
Dim rstProducts As DAO.Recordset
Set rstProducts = dbsNorthwind.OpenRecordset(SQL, dbOpenSnapshot)
If rstProducts.EOF Then Exit Sub
SQL = "SELECT CategoryID, CategoryName FROM Categories " & _
"ORDER BY CategoryID"
Dim rstCategories As DAO.Recordset
Set rstCategories = dbsNorthwind.OpenRecordset(SQL, dbOpenSnapshot)
' For each category find the product generating the least revenue
' and the product generating the most revenue.
Do Until rstCategories.EOF
Dim Criteria As String
Criteria = "CategoryID = " & rstCategories![CategoryID]
rstProducts.FindFirst Criteria
Dim HighRev As Currency
HighRev = rstProducts![UnitPrice] * rstProducts![UnitsOnOrder]
If Not rstProducts.NoMatch Then
' Set bookmarks at the first record containing the CategoryID.
Dim FirstMark As Variant, HighMark As Variant, LowMark As Variant
FirstMark = rstProducts.Bookmark
HighMark = FirstMark
LowMark = FirstMark
' Find the product generating the most revenue.
Do While rstProducts![CategoryID] = rstCategories![CategoryID]
If rstProducts![UnitPrice] * rstProducts![UnitsOnOrder] > _
HighRev Then
HighRev = rstProducts![UnitPrice] * _
rstProducts![UnitsOnOrder]
HighMark = rstProducts.Bookmark
End If
rstProducts.MoveNext
Loop
' Move to the first record containing the CategoryID.
rstProducts.Bookmark = FirstMark
Dim LowRev As Currency
LowRev = rstProducts![UnitPrice] * rstProducts![UnitsOnOrder]
' Find the product generating the least revenue.
Do While rstProducts![CategoryID] = rstCategories![CategoryID]
If rstProducts![UnitPrice] * rstProducts![UnitsOnOrder] < _
LowRev Then
LowRev = rstProducts![UnitPrice] * _
rstProducts![UnitsOnOrder]
LowMark = rstProducts.Bookmark
End If
rstProducts.MoveNext
Loop
End If
' Set high and low bookmarks to build the message ing.
Dim Message As String
Message = "CATEGORY: " & rstCategories!CategoryName & _
vbCrLf & vbCrLf
rstProducts.Bookmark = HighMark
Message = Message & "HIGH: $" & HighRev & " " & _
rstProducts!ProductName & vbCrLf
rstProducts.Bookmark = LowMark
Message = Message & "LOW: $" & LowRev & " " & _
rstProducts!ProductName
MsgBox Message, , "Product Statistics"
rstCategories.MoveNext
Loop
rstProducts.Close
rstCategories.Close
dbsNorthwind.Close
Set rstProducts = Nothing
Set rstCategories = Nothing
Set dbsNorthwind = Nothing
Exit Sub
ErrorHandler:
MsgBox "Error #: " & Err.Number & vbCrLf & vbCrLf & Err.Description
End SubI Get By With a Little Help From My Compiler
Since we are all using Option Explicit around here (that's a statement, not a question), we gain some additional help from the compiler by declaring our variables closer to where we use them.
Mike Wolfe
With Option Explicit in place, our code will not compile if we try to use a variable before we declare it. The declaration line must precede its first use. Perhaps that's why people started declaring variables at the top of procedures in the first place.
But refactoring your code to avoid compile errors is counterproductive. It's not so much that compile errors are good, it's just that compile errors are way better than runtime or logic errors.
Suppose we have some code that uses a counter to break out of a loop:
Sub MyProcedure()
'Do a bunch of stuff
Dim i As Long
Do Until i >= MaxLoops
i = i + 1
'Do some other stuff
Loop
End SubIn the above code, we don't explicitly initialize i, so it gets the default Long value of 0.
If, years later, we try to initialize i to something other than zero in the portion of the code where we are "doing a bunch of stuff," we will get a compile error, "Variable not defined":
Sub MyProcedure()
'Do a bunch of stuff
i = 42
'Keep doing stuff
Dim i As Long
Do Until i >= MaxLoops
i = i + 1
'Do some other stuff
Loop
End SubA compile error in this situation is good! It alerts us to the fact that we are doing something unexpected. It's easy to resolve the compile error: either we move the Dim i As Long line up above the line of code that sets i = 42 or we create a new variable to hold the secret to life, the universe, and everything (i.e., 42).
On the other hand, if we had declared Dim i As Long in the procedure header (following Microsoft's poor example), this unexpected code change could have easily gone unnoticed. At best it would have been a runtime error. At worst, it would have been a festering logic error.
