|
|
I'm in the process of updating an Excel spreadsheet that is failing when it is running inside of Internet Explorer. The issue is related to the ActiveSheet and other global properties having a value of Nothing when the code is assuming they have valid references. As I am going through this spreadsheet, I am noting a wide variety of programming deficiencies and inefficiencies. Here is a list of some of the issues encountered: If you run spreadsheets under Internet Explorer, use Application.ThisWorkbook and to ensure the browser evaluates the reference correctly. NOTE: This is not an exhaustive list, nor do I claim that the "Better" examples are the best code example, but rather they are (hopefully) significant improvements over the "Bad" examples. In many cases, I have kept the design of the bad example so that you can see the differences however you should strive to completely refactor the code if possible. -
Do not abuse the "With" statement in VBA. Some people like "With" and some people don't and I don't particularly care for it. In particular, do not use nested "With" statements. Example of Bad Code ' ... BAD EXAMPLE: DO NOT USE ...
With ActiveSheet
With TestForm
' ...
End With ' TestForm
End With ' ActiveSheet
' ... BAD EXAMPLE: DO NOT USE ...
-
Avoid using the "Goto" statement in VBA.
"Goto" is almost always a bad idea and a sign of spaghetti code. There are very few cases were it is appropriate to use it -- very few cases!
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
For i = iHere To lRealLastRow Step 2
BuildStr = "A" & i + 1
Range(BuildStr).Select
If Range(BuildStr).Value = "---" Then GoTo DoneWithNames
SelectItemDlg.Items_List.AddItem Range(BuildStr).Text
Next
DoneWithNames:
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code (I still would have designed it differently)
' ...
Dim MyCell As Range
For i = iHere To lRealLastRow Step 2
Set MyCell = Cells(i + 1, 1)
If MyCell.Value = "---" Then
Exit For
End If
SelectItemDlg.Items_List.AddItem MyCell.Text
Next
' ...
-
Avoid using the name of a Form inside the Form code
The current form is implied in the code. It is not necessary to use the form name to reference controls on the form. If you want to differeniate form controls, you can use the 'Me' keyword such as: Me.Hide
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
' This example is doubly bad because it uses the form name (ChartConfig)
' both explicitly and in a With statement
If ChartConfig.Variable1.Text = ChartConfig.Variable2.Text Then
MsgBox "Variable1 cannot be same as Variable2. Please choose another variable type.", vbOKCancel
'Turn off the Change effect on Variable2.
changeCasevar = False
With ChartConfig
.Variable2.Text = ""
.Variable2Desc.Text = ""
.Variable2Uom.Text = ""
End With
'Turn back on the Change effect on Variable2.
changeCasevar = True
Exit Sub
'
End If
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code (I still would have designed it differently)
' ...
If Variable1.Text = Variable2.Text Then
MsgBox "Variable1 cannot be same as Variable2. Please choose another variable type.", vbOKCancel
'Turn off the Change effect on Variable2.
changeCasevar = False
Variable2.Text = ""
Variable2Desc.Text = ""
Variable2Uom.Text = ""
'Turn back on the Change effect on Variable2.
changeCasevar = True
Exit Sub
End If
-
Use Error Handling and Resume to restore Application.ScreenUpdating
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
' There are quite a few global variables and a pass-by-reference
' boolean that should just be the function return value.
Application.ScreenUpdating = False
iOption = "Single"
x = ChartPicker.Input_Val_Text
Call CheckUserRangeInputsAndOutputs(iOption, OkToChart)
iOption = ""
If OkToChart = True Then
With ChartConfig
CreateChartingArrays
End With
End If
Application.ScreenUpdating = True
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code (I still would have designed it differently)
' ...
On Error GoTo ErrorHandler
Application.ScreenUpdating = False
If CheckUserRangeInputsAndOutputs("Single") Then
ChartConfig.CreateChartingArrays
End If
Exit_Handler:
Application.ScreenUpdating = True
Exit Sub
ErrorHandler:
MsgBox "Error " & Err.Number & ": " & Err.Description
Resume Exit_Handler
-
Use If, Else If, and Else correctly.
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
If TextBoxMinimum.Text = "" Then
MsgBox "Must enter Minimum Value before proceeding", vbOK
Result = False
Exit Function
End If
If TextBoxMaximum.Text = "" Then
MsgBox "Must enter Maximum Value before proceeding", vbOK
Result = False
Exit Function
End If
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code
' ...
' For a explicit method, you can directly refer to the controls
Result = False
If Not IsNumeric(TextBoxMinimum.Text) Then
MsgBox "Must enter Minimum Value before proceeding", vbOK
TextBoxMinimum.SetFocus
ElseIf Not IsNumeric(TextBoxMaximum.Text) Then
MsgBox "Must enter Maximum Value before proceeding", vbOK
TextBoxMaximum.SetFocus
Else
Result = True
End If
' If you can use a generic method, you can iterate through all of the controls
' There are obviously better ways of doing this, especially with .NET
Dim ValidateControl
Dim ControlList = Array(TextBox1, TextBox2, TextBox3)
For Each ValidateControl In ControlList
If Input_Val_Text = Null Or Input_Val_Text = "" Then
MsgBox "Must enter Value in " & ValidateControl.Name & " before proceeding", vbOK
Result = False
Exit Function
End If
Next
-
Avoid using the ActiveSheet, Range, Cells, and other global variables.
Rather than assuming a position, it is much better to set a variable to the desired
Workbook, Worksheet, or Range.
In particular, if there is any chance the spreadsheet might run under a browser,
use Application.ThisWorkbook to ensure the browser
evaluates the macro correctly.
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
'NOTE: Arrays start with 1st position as "0", not "1"
'Thus ColLtrs(0) is not used but placed there as a spacer for letter to column alignment:
ColLtrs = Array("", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z")
With ActiveSheet
Range(ColLtrs(iOutCol) & iOutRow).Select
If UnitSystem = "Metric" Then
iRow = ActiveCell.Row
For i = iRow To iLastRow - 1
currCell = ColLtrs(ActiveCell.Column) & i + 1
If Range(currCell).Value <> "" Then
'capture the value from original calculatated data
Range(currCell).Select
strOrigValueCell = ColLtrs(iResultsCol) & ActiveCell.Row
Result = ConvertUom(Range(strOrigValueCell))
End If
Next i
End If
End With
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code
' ...
Dim mySheet As Worksheet
Dim Value As Variant
Set mySheet = ActiveSheet
For i = iRow To iLastRow - 1
Value = mySheet.Cells(i, iOutCol).Value
If Value <> "" Then
Result = ConvertUom(UnitSystem, Value)
End If
Next i
' To get a column letter/code, do not reinvent the wheel!
' Various functions from:
' http://www.dicks-blog.com/archives/2004/05/21/column-numbers-to-letters/
Function ColLetter(ColNumber As Long) As String
On Error Resume Next
ColLetter = Application.Substitute(Application.ConvertFormula("R1C" & ColNumber, xlR1C1, xlA1, 4), "1", "")
End Function
Function ColumnLetter(ByVal c As Long) As String
Dim p As Long
While c
p = 1 + (c - 1) Mod 26
c = (c - p) \ 26
ColumnLetter = Chr$(64 + p) & ColumnLetter
Wend
End Function
-
Avoid using .Select and moving selection in macros unless absolutely.
Do not change the current sheet or selection unless that is the intention of the macro.
In general, don't mess with the user, flip sheets, or other heinous acts and
try to leave things the way you found them.
Example of Bad Code
' ... BAD EXAMPLE: DO NOT USE ...
For i = 1 To 10
Range("A" & i).Select
ActiveCell.Value = "Row " & i
Next i
' ... BAD EXAMPLE: DO NOT USE ...
Example of Better Code
Dim iColumn As Long
Dim mySheet As Worksheet
Dim myRange As Range
iColumn = 1
Set mySheet = ActiveSheet
For iRow = 1 To 10
Set rng = mySheet.Cells(iRow, iColumn)
rng.Value = "Row " & iRow
Next iRow
|