Userform for getting data from data sheet into a table

I am making a userform that grabs data from a data sheet and puts it into a table:

• Grabs the data based on what the user wants (Brand -> Items for brand)

• Allows multiple items to be added

• Displays the info about the items

• Allows the user to specify how many of each item (for when the data is put into the inventory table)

I am just looking for any suggestions on how I could make my code better, specifically with error handling and lowering memory footprint. I am a novice, so some of this code could have better approaches. If so, please tell me.

UserForm:

ThisWorkBook:

Private Sub Workbook_BeforeClose(Cancel As Boolean)
Application.AutoCorrect.AutoFillFormulasInLists = True
End Sub

Private Sub Workbook_Open()
Application.AutoCorrect.AutoFillFormulasInLists = False
End Sub

Add Item Button:

Private Sub cbAddItemUserForm_Click()

ufItemAdd.Show

End Sub

UserForm:

```
Public brand_edit As Variant
Public cBook As Workbook
Public cSheet As Worksheet
Public dSheet As Worksheet
Public specLink As Variant
Public itemAddress As Variant
Public itemID As String
Public inventoryTable As ListObject
Public x As Long
Public quantity As String

Private Sub MultiPage1_Change()

End Sub

Private Sub UserForm_Activate()

Me.tbQuantity.Text = "1"

End Sub

Public Sub cmbBrand_Change()

Me.tbQuantity.Text = "1"

Dim brand As Variant
brand = cmbBrand.Value
brand_edit = Replace(brand, " ", "_")
brand_edit = Replace(brand_edit, """", "")
brand_edit = Replace(brand_edit, "-", "")
brand_edit = Replace(brand_edit, "(", "")
brand_edit = Replace(brand_edit, ")", "")
brand_edit = Replace(brand_edit, "&", "and")
brand_edit = Replace(brand_edit, ".", "")
brand_edit = Replace(brand_edit, ",", "")
brand_edit = Replace(brand_edit, ", ", "_")
brand_edit = Replace(brand_edit, "__", "_")
brand_edit = LCase(brand_edit)

'On Error Resume Next
'If brand_edit = "" Then
' cmbItemID.RowSource = ""
'Else

On Error Resume Next

If Err = 380 Then
Exit Sub
Else
cmbItemID.R

Option Explicit

Go to Tools -> Options -> Require Variable Declaration. This will insert Option Explicit at the top of every new module you create.
Option Explicit will enforce that every variable you use is declared. This will prevent all sorts of bugs, mainly due to preventing typos.

Separation of concerns

You should never have business logic contained directly in your event triggers. It means your code is scattered around, is not easily find-able, and is very tightly coupled with your form. You should separate out your logic into Sub/Functions which your event handlers can then call.

Take this for instance:

Public Sub cmbBrand_Change()

Me.tbQuantity.Text = "1"

Dim brand As Variant
brand = cmbBrand.Value
brand_edit = Replace(brand, " ", "_")
brand_edit = Replace(brand_edit, """", "")
brand_edit = Replace(brand_edit, "-", "")
brand_edit = Replace(brand_edit, "(", "")
brand_edit = Replace(brand_edit, ")", "")
brand_edit = Replace(brand_edit, "&", "and")
brand_edit = Replace(brand_edit, ".", "")
brand_edit = Replace(brand_edit, ",", "")
brand_edit = Replace(brand_edit, ", ", "_")
brand_edit = Replace(brand_edit, "__", "_")
brand_edit = LCase(brand_edit)

That whole brand_edit thing should be a Function. Maybe something called CleanBrandName which takes a brandName as an argument and returns a cleaned version:

Public Function CleanBrandName(ByVal brandName As String) As String

    Dim cleanName  As String
    cleanName = brandName

    cleanName = Replace(cleanName, " ", "_")
    cleanName = Replace(cleanName, ", ", "_")
    cleanName = Replace(cleanName, "__", "_")

    cleanName = Replace(cleanName, """", "")
    cleanName = Replace(cleanName, "-", "")
    cleanName = Replace(cleanName, "(", "")
    cleanName = Replace(cleanName, ")", "")
    cleanName = Replace(cleanName, ".", "")
    cleanName = Replace(cleanName, ",", "")

    cleanName = Replace(cleanName, "&", "and")

    cleanName = LCase(cleanName)

    CleanBrandName = cleanName

End Function

And now your cmbBrand_Change can just go:

Public Sub cmbBrand_Change()

Me.tbQuantity.Text = "1"

Dim brand As Variant
brand = cmbBrand.Value
brand = CleanBrandName(brand)

And when you find new cases that need to be handled by CleanBrandName you know where to find it and that you only need to change it in that one place. If you ever need to clean a brand name somewhere else in your code, you can just call that function rather than copy-pasting all the logic again.

Keep your code tidy and organised

'On Error Resume Next
'If brand_edit = "" Then
'    cmbItemID.RowSource = ""
'Else

On Error Resume Next

If Err = 380 Then
    Exit Sub
Else
cmbItemID.RowSource = brand_edit

End If

Err.Clear

On Error GoTo 0

cmbItemID.Text = ""

This is just a mess. Don't leave commented-out code in your codebase, get yourself some proper Source Control (I highly recommend RubberDuck which is an Add-In that provides, among other awesome things, Git Integration for the VBE).

On Error Resume Next is a very dangerous command that should be avoided wherever possible and, if not, then used under tightly-defined circumstances.

This is an appropriate way to use it:

itemValue = empty

'/ Will error if the key does not exist
On Error Resume Next
    itemValue = collection.Item(key)
On Error Goto 0

If not IsEmpty(itemValue) Then
    '/ Key Exists, Do Stuff
Else
    '/ Handle missing Key
End If

We have a statement which may cause an error, so we temporarily disable error handling for that statement, immediately re-enable it afterwards and immediately handle the error if it occurred. And we leave a comment explaining what error we were expecting and why we're doing things this way.

Back to your code, this is pointless:

On Error Resume Next

If Err = 380 Then
    Exit Sub
Else

If an error had already occurred before On Error... then the program would've stopped already and thrown an error message, so checking after the fact is not useful.

What on earth is this doing?

cmbItemID.RowSource = brand_edit

.RowSource is supposed to be used to set a valid Range Reference, like say "A10" or "NamedRange". How a brandName fits in to that is incredibly unclear.

Err.Clear

On Error GoTo 0

If you're expecting an error, then handle the error you're expecting. Don't just dismiss any error whatsoever. Errors are important. Errors are useful. Errors should not be ignored unless you can articulate precisely what you're ignoring and why.

cmbItemID.Text = ""

End Sub

And you're just clearing the box at the end of the sub anyway? So why on earth have this change event in the first place?

This is why explanatory comments are important. If your code is doing something, and the thing is not immediately obvious, you should leave a comment explaining what's going on and why.

StackExchange Code Review Q#132257, answer score: 6