patternMinor
Structure for Multiple Potential Find Errors
Viewed 0 times
findforstructuremultipleerrorspotential
Problem
I have been writing some code to add to our company's Bill of Materials template Excel file. Every project that we do has its own unique Bill of Materials. I am attempting to make it more dynamic and to add some functionality that it didn't previously have. I have created an add-in
The add-in then contains the following code:
```
'Require all variables to be defined within the sub.
Option Explicit
Sub WorksheetSelectionChange(wb As Workbook, ws As Worksheet, ByVal Target As Range)
'This sub will run whenever the selection is changed on this sheet. It will
'check if any of the required headings have been deleted and prompt the user to
'add the heading back if it has. It will also check if the selection is in the
'DOC or PO ATTACHMENTS columns and proceed accordingly.
Dim ErrorNo As Long
Dim HeaderRow As Long
Dim POAttachmentsColumn As Long
Dim POFilesColumn As Long
Dim CodeColumn As Long
Dim QTYColumn As Long
Dim DescriptionColumn As Long
Dim CostColumn As Long
Dim LastRow As Long
Dim YNAnswer As Integer
Dim DOCColumn As Long
Dim Option1Row As Long
Dim AdderDeductColumn As Long
Dim OptionTotalRow As Long
Dim Option1RowCount As Long
Dim i As Long
'Disable screen updating if it is currently enabled.
If Not (Application.ScreenUpdating = False) Then Application.ScreenUpdating = False
'Go to Error_Handling on an error. Check if any of the required headings have been
'd
BOM MACROS.xlam that each user will add, and stored it on the company network so that I can modify the code and push it out to every file at once. As such, each file will contain the following code within the main worksheet:'Require all variables to be defined within the sub.
Option Explicit
Private Sub Worksheet_SelectionChange(ByVal Target As Range)
Application.Run "'BOM MACROS.xlam'!" & "WorksheetSelectionChange", ActiveWorkbook, ActiveSheet, Target
End SubThe add-in then contains the following code:
```
'Require all variables to be defined within the sub.
Option Explicit
Sub WorksheetSelectionChange(wb As Workbook, ws As Worksheet, ByVal Target As Range)
'This sub will run whenever the selection is changed on this sheet. It will
'check if any of the required headings have been deleted and prompt the user to
'add the heading back if it has. It will also check if the selection is in the
'DOC or PO ATTACHMENTS columns and proceed accordingly.
Dim ErrorNo As Long
Dim HeaderRow As Long
Dim POAttachmentsColumn As Long
Dim POFilesColumn As Long
Dim CodeColumn As Long
Dim QTYColumn As Long
Dim DescriptionColumn As Long
Dim CostColumn As Long
Dim LastRow As Long
Dim YNAnswer As Integer
Dim DOCColumn As Long
Dim Option1Row As Long
Dim AdderDeductColumn As Long
Dim OptionTotalRow As Long
Dim Option1RowCount As Long
Dim i As Long
'Disable screen updating if it is currently enabled.
If Not (Application.ScreenUpdating = False) Then Application.ScreenUpdating = False
'Go to Error_Handling on an error. Check if any of the required headings have been
'd
Solution
If Not (Application.ScreenUpdating = True) Then Application.ScreenUpdating = TrueThat is quite ugly. First, don't ever compare a boolean value to a boolean literal - just use the value in the boolean, like this:
If Not (Application.ScreenUpdating) Then Application.ScreenUpdating = TrueNext, do you even really need this check? This check is usually only done if the potential changes will potentially cause major updates, like recalculating many values and redrawing the entire screen based on an update. In fact, the only place I usually see checks like this is when updating a field that calls
OnNotifyPropertyChanged in C# (which does exactly what I just described). You should probably just assign the value like this:Application.ScreenUpdating = TrueIn fact, I would not be surprised if
Application.ScreenUpdating did the check internally if the change potentially causes major calculations. My guess is, however, that you are just assigning a flag that other methods check as needed.Next, check this cool string formatter: A CSharpish String.Format formatting helper. This will help you clean up that massive
if case by calculating a couple error-specific values, then outputting with a single output statement.You don't need to line up the declarations. Some people think it looks neat, I personally do not.
Rubberduck 2.0 (still in development as of this writing) says you don't need to use
"" in your comparison, and you should use vbNullString instead.It also says you never use the parameter
wb, and that you should explicitly pass your parameters ByRef. However, because you never assign them, it says you can pass both wb and ws by value. The point about explicitly stating what the modifier is also applies to WorksheetSelectionChange, which is implicitly public.Code Snippets
If Not (Application.ScreenUpdating = True) Then Application.ScreenUpdating = TrueIf Not (Application.ScreenUpdating) Then Application.ScreenUpdating = TrueApplication.ScreenUpdating = TrueContext
StackExchange Code Review Q#117303, answer score: 4
Revisions (0)
No revisions yet.