HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Structure for Multiple Potential Find Errors

Submitted by: @import:stackexchange-codereview··
0
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 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 Sub


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

Solution

If Not (Application.ScreenUpdating = True) Then Application.ScreenUpdating = True


That 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 = True


Next, 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 = True


In 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 = True
If Not (Application.ScreenUpdating) Then Application.ScreenUpdating = True
Application.ScreenUpdating = True

Context

StackExchange Code Review Q#117303, answer score: 4

Revisions (0)

No revisions yet.