Simplifying manual data entry with a UserForm Excel 2010

Question

I am an absolute beginner. This is the first thing I've ever coded in VBA. I don't know if I'm allowed to ask for just criticism in general without a question.

But just in case I can't, this file is going to get very large and I imagine I will have resource optimization problems - so what should I have done differently to compute these tasks efficiently?

The purpose of this code is to streamline some manual data entry I'm having to do at work. Basically, my department has a recruiting function and we post jobs on various websites. In order to develop some kind of metric, it is my job to record the performance data of jobs posted by all recruiters in department.

A sample of the table

I've created this form in order to put some of this work back on the recruiters as part of their job posting workflow.

Public reqnum As String
Public jbtitle As String
Public jbloc As String
Public postdate As String
Public closedate As String
Public vws As Integer
Public apps As Integer
Public jobcategory As String
Public pidname As String
'If someone hits enter on the last textbox, it will submit the form
Private Sub ApplicantsBox_KeyDown(ByVal KeyCode As MSForms.ReturnInteger, ByVal Shift As Integer)
 If KeyCode = 13 Then
 Call UpdateButton_Click
 End If
End Sub
'If someone hits enter while focused on the check box, they will change its value
Private Sub GoToCheckbox_KeyDown(ByVal KeyCode As MSForms.ReturnInteger, ByVal Shift As Integer)
 If KeyCode = 13 Then
 If GoToCheckbox.Value = True Then
 GoToCheckbox.Value = False
 Else
 GoToCheckbox.Value = True
 End If
 End If
End Sub
Private Sub UpdateButton_Click()
 'Establish variables with textbox values
 reqnum = ReqNumBox.Value
 jbtitle = JobTitleBox.Value
 jbloc = JobLocationBox.Value
 postdate = DatePostedBox.Value
 closedate = DateClosedBox.Value
 jobcategory = CategoryComboBox.Value
 pidname = PIDComboBox.Value
 'Check to make sure data in views and applications is a number
 If Not IsNumeric(ViewsBox.Value) Then
 MsgBox ("You put in a non-number for Views")
 Exit Sub
 Else
 vws = ViewsBox.Value
 End If
 If Not IsNumeric(ApplicantsBox.Value) Then
 MsgBox ("You put in a non-number for Applications")
 Exit Sub
 Else
 apps = ApplicantsBox.Value
 End If
 'Check to see if the user selected other as either a job category or name
 Call AddToDirectory
 'Decision Tree for a search by either reqnum or job title, location and post date
 If Not reqnum = "" Then
 Dim reqnum_row As Long
 reqnum_row = 0
 'Search mechanism for requisition numbers
 For Each c In Sheets(1).Range("A:A")
 If c.Value = reqnum Then
 reqnum_row = c.row
 End If
 Next c
 'if there isn't an exisiting req...
 If reqnum_row = 0 Then
 'Set a variable as the last row with text
 Dim rw As Long
 rw = LastRowInOneColumn("D", 1)
 'Establish the first empty row
 Dim newrow As Long
 newrow = rw + 1
 'Add information to row
 Call UpdateCellData(newrow)
 'Format the row with alternating background color
 Call FormatRows(newrow)
 Call CloseForm(newrow)
 Else
 Call UpdateCellData(reqnum_row)
 Call CloseForm(reqnum_row)
 End If
 Else
 'Logic for if there is not a reqnum
 Dim job_row As Long
 job_row = 0
 'Search mechanism for search by job title then location then post date.
 'If all given data equals all the same in a row it will be selected as the row to update
 For Each X In Sheets(1).Range("B:B")
 If X.Value = jbtitle Then
 If X.Offset(0, 1).Value = jbloc Then
 If X.Offset(0, 2).Value = postdate Then
 job_row = X.row
 End If
 End If
 End If
 Next X
 'If the search comes up empty add a new row of data
 If job_row = 0 Then
 'Set a variable as last row with text
 Dim rwj As Long
 rwj = LastRowInOneColumn("D", 1)
 'set a variable as first row without text
 Dim newrowj As Long
 newrowj = rwj + 1
 Call UpdateCellData(newrowj)
 'Formatting the rows
 Call FormatRows(newrowj)
 Call CloseForm(newrowj)
 Else
 Call UpdateCellData(job_row)
 Call CloseForm(job_row)
 End If
 End If
End Sub
Function LastRowInOneColumn(column As String, ws As Long) As Long
'Find the last used row in a Column: column A in this example
 Dim lastrow As Long
 With Sheets(ws)
 lastrow = .Cells(.Rows.Count, column).End(xlUp).row
 End With
 LastRowInOneColumn = lastrow
End Function
Sub UpdateCellData(n As Long)
 'Add information to the rows
 'if the textbox is blank or is zero, do nothing.
 With Sheets(1)
 If Not reqnum = "" Or reqnum = " " Then
 .Cells(n, 1).Value = reqnum
 End If
 If Not jbtitle = "" Or jbtitle = " " Then
 .Cells(n, 2).Value = jbtitle
 End If
 If Not jbloc = "" Or jbloc = " " Then
 .Cells(n, 3).Value = jbloc
 End If
 If Not postdate = "" Or postdate = " " Then
 .Cells(n, 4).Value = postdate
 End If
 If Not closedate = "" Or closedate = " " Then
 .Cells(n, 5).Value = closedate
 End If
 If Not vws = 0 Then
 .Cells(n, 7).Value = vws
 End If
 If Not apps = 0 Then
 .Cells(n, 8).Value = apps
 End If
 If Not jobcategory = "<Update>" Then
 .Cells(n, 11) = jobcategory
 End If
 If Not pidname = "<Update>" Then
 .Cells(n, 12) = pidname
 End If
 End With
End Sub
Function IsEven(z As Long) As Boolean
 If z Mod 2 = 0 Then
 IsEven = True
 Else
 IsEven = False
 End If
End Function
Sub CloseForm(w As Long)
 If GoToCheckbox.Value = True Then
 Sheets(1).Activate
 Sheets(1).Rows(w).Select
 Unload Me
 Else
 With ReqNumBox
 .SetFocus
 .SelStart = 0
 .SelLength = Len(.Text)
 End With
 End If
End Sub
Private Sub UpdateButton_KeyDown(ByVal KeyCode As MSForms.ReturnInteger, ByVal Shift As Integer)
 If KeyCode = 13 Then
 Call UpdateButton_Click
 End If
End Sub
Sub FormatRows(rowtoformat As Long)
 'Extend Formulas
 With Sheets(1)
 .Range("F3:F" & rowtoformat).FillDown
 .Range("I3:I" & rowtoformat).FillDown
 .Range("J3:J" & rowtoformat).FillDown
 End With
 'Format Rows
 Dim even As Boolean
 even = IsEven(rowtoformat)
 If even = True Then
 Sheets(1).Rows(4).Copy
 Sheets(1).Rows(rowtoformat).PasteSpecial Paste:=xlFormats
 Else
 Sheets(1).Rows(5).Copy
 Sheets(1).Rows(rowtoformat).PasteSpecial Paste:=xlFormats
 End If
End Sub
Private Sub UserForm_Initialize()
 'Find the last row in column a
 a = LastRowInOneColumn("A", 2)
 'Iterate through all PI Data options
 For Each z In Worksheets("Hidden Data Storage").Range("A2:A" & a)
 With PIDComboBox
 .AddItem (z)
 End With
 Next z
 b = LastRowInOneColumn("B", 2)
 For Each w In Worksheets("Hidden Data Storage").Range("B2:B" & b)
 With CategoryComboBox
 .AddItem (w)
 End With
 Next w
End Sub
Sub AddToDirectory()
 'Checks to see if the person selected "Other" as the value for either PID or Category
 If pidname = "Other" Then
 Dim newname As String
 newname = InputBox("Please add your name: ")
 'adds new name to directory
 With Worksheets("Hidden Data Storage")
 Dim v As Long
 v = LastRowInOneColumn("A", 2)
 Dim vadd As Long
 vadd = v + 1
 .Cells(vadd, 1).Value = newname
 End With
 End If
 'Checks to see if "other" is selected as job category
 If jobcategory = "Other" Then
 Dim newcategory As String
 'asks for new category
 newcategory = InputBox("Please add the new category: ")
 'adds new category to the hidden directory
 With Worksheets("Hidden Data Storage")
 Dim h As Long
 h = LastRowInOneColumn("B", 2)
 Dim hadd As Long
 hadd = h + 1
 .Cells(hadd, 2).Value = newcategory
 End With
 End If
End Sub

Answer 1

Very fine effort and I see very little to change for efficiency reasons.

If it does start to slow down during your updates, you could turn off Application.ScreenUpdating and turn it back on after the UserForm has initialized.

But I don't see that as an issue now.

Some small suggestions below - I don't think they will do much for speed - but address readibility, maintenance and efficiency.

Improve readability by using predefined and system constants

EDIT - use constant at top for Carriage Return
Public Const CR As Integer = 13

Then Replace all checks for
KeyCode = 13 with KeyCode = CR

Simplify

Change:
If GoToCheckbox.Value = True Then GoToCheckbox.Value = False Else GoToCheckbox.Value = True End If

To (just make sure you have default value = 0)
GoToCheckbox.Value = Not GoToCheckbox.Value

Practice common or even standard coding practice

• Move your Dims to top of function/sub code

Simplify logical comparisons

• no need to compare to true - or even to use a variable if you have a well-named function that indicates it returns true/false

• continue to reference Sheets using With

Example:

Change

Sub FormatRows(rowtoformat As Long)
 'Extend Formulas
 With Sheets(1)
 .Range("F3:F" & rowtoformat).FillDown
 .Range("I3:I" & rowtoformat).FillDown
 .Range("J3:J" & rowtoformat).FillDown
 End With
 'Format Rows
 Dim even As Boolean
 even = IsEven(rowtoformat)
 If even = True Then
 Sheets(1).Rows(4).Copy
 Sheets(1).Rows(rowtoformat).PasteSpecial Paste:=xlFormats
 Else
 Sheets(1).Rows(5).Copy
 Sheets(1).Rows(rowtoformat).PasteSpecial Paste:=xlFormats
 End If
End Sub

To

Sub FormatRows(rowtoformat As Long)
 'Extend Formulas
 With Sheets(1)
 .Range("F3:F" & rowtoformat).FillDown
 .Range("I3:I" & rowtoformat).FillDown
 .Range("J3:J" & rowtoformat).FillDown
 'Format Rows 
 If IsEven(rowtoformat) Then
 .Rows(4).Copy
 Else
 .Rows(5).Copy
 End If
 .Rows(rowtoformat).PasteSpecial Paste:=xlFormats
 End With
End Sub

Hope that helps

• Brad Johansen: Thank you for the improvements. What is vbCr?
• dbmitch: It's a VBA constant. Oops - that probably won't work - I just realized it's not 13 - it's CHR(13) - the actual ASCII key - I'll update my edit to use a constant at the top instead

Answer 2

Performance

"I imagine I will have resource optimisation problems"

You'll be fine. To repeat a widespread programming mantra:

"Premature optimisation is the root of all evil"

Just build your thing, and build it well, and if you encounter performance problems down the line, then start worrying about optimising for performance.

That aside, some general VBA performance tips:

Application.ScreenUpdating
Application.EnaleEvents
Application.Calculation
Application.StatusBar

If your application doesn't need events to fire, or doesn't need formulas to recalculate during execution, or your user doesn't need to see what's going on, you can turn those settings off as appropriate during the start of a Sub/Function and reset it at the end.

ScreenUpdating in particular will generally makes a huge difference.

---

Naming

Your naming is actually pretty decent, but could still be better.

Names should be Descriptive, then Unambiguous and only then concise. Your names should be written to be understood as easily as possible. Not read quickly, understood quickly.

jbtitle vs jobTitle

It takes just one additional character to transform it from an abbreviation I have to decode, to a name I can just read.

jbloc vs jobLocation

Again, takes a fraction of a second longer to read, is much easier to understand.

vws

What on earth is that? I have no idea. I had to go hunting through your code to figure it out. Just call it viewBoxValue and you'll never have a problem understanding what it is and what it's doing in your code.

apps vs numApplicants

Again, fractionally longer to read, so much easier to understand what's going on.

pidname

I don't actually know what these PID values are meant to represent. It's not in your code. I recommend something more descriptive.

Magic Numbers

If KeyCode = 13 Then

What is the significance of 13? Make it an appropriately named constant and you'll never need to look it up further down the line.

Public Const ENTER_KEY_KEYCODE As Long = 13
...
If keyCode = ENTER_KEY_KEYCODE Then

And now your code is completely self-explanatory.

This rule applies to any hard-coded value that appears in your code. Whenever you see a number, or a string, or anything hardcoded, ask yourself igf it couldn't be put in a variable instead.

Naming Conventions

Naming Conventions are incredibly useful, when applied consistently, because they allow you to embed metaData in your code and to easily see unusual variables / potential errors.

Standard VBA conventions go thus:

Local Variables: Written in camelCase.

Dim localVariable As String
includes method arguments.

Module / Global Variables: Written in PascalCase.

Private ModuleVariable As String
Public GlobalVariable As Long

Method Names: Verbs. Written in PascalCase

Private Function ReturnThisValue() As Long
Public Sub DoThisThing()

Constants: Written in SHOUTY_SNAKE_CASE

Public Const CONSTANT_VALUE As String = "This Value Never Changes"

---

Refactoring

Big things are made up of lots of little things. Little things are made up of even smaller things. The art of programming is taking one big thing, and splitting it up into many different steps which, when brought together, give you the result you want.

If you ever find yourself writing a comment that says what your code is doing

'Check to make sure data in views and applications is a number

that's a good sign that the thing should be put into its' own, appropriately named, Sub/Function.

---

'If someone hits enter while focused on the check box, they will change its value
Private Sub GoToCheckbox_KeyDown(ByVal KeyCode As MSForms.ReturnInteger, ByVal Shift As Integer)
 If KeyCode = 13 Then
 If GoToCheckbox.Value = True Then
 GoToCheckbox.Value = False
 Else
 GoToCheckbox.Value = True
 End If
 End If
End Sub

First off, that comment, if it should be anywhere, should be inside the function it refers to. Using our previous point about Constants and a little simplification to do with Boolean values, we can change it to this:

Private Sub GoToCheckbox_KeyDown(ByVal KeyCode As MSForms.ReturnInteger, ByVal Shift As Integer)
 If KeyCode = ENTER_KEY_KEYCODE Then
 With gotocheckbox
 .Value = Not .Value
 End With
 End If
End Sub

And now the comment is pretty redundant because the code quite literally says what it does.

---

Public Function IsEven(z As Long) As Boolean
 If z Mod 2 = 0 Then
 IsEven = True
 Else
 IsEven = False
 End If
End Function

This can be similarly simplified

Public Function IsEven(ByVal checkValue As Double) As Boolean
 IsEven = (checkValue Mod 2 = 0)
End Function

---

And now we get to something we can really sink our teeth into.

Private Sub UpdateButton_Click()
 'Establish variables with textbox values
 reqnum = ReqNumBox.Value
 jbtitle = JobTitleBox.Value
 jbloc = JobLocationBox.Value
 postdate = DatePostedBox.Value
 closedate = DateClosedBox.Value
 jobcategory = CategoryComboBox.Value
 pidname = PIDComboBox.Value
 'Check to make sure data in views and applications is a number
 If Not IsNumeric(ViewsBox.Value) Then
 MsgBox ("You put in a non-number for Views")
 Exit Sub
 Else
 vws = ViewsBox.Value
 End If
 If Not IsNumeric(ApplicantsBox.Value) Then
 MsgBox ("You put in a non-number for Applications")
 Exit Sub
 Else
 apps = ApplicantsBox.Value
 End If
 'Check to see if the user selected other as either a job category or name
 Call AddToDirectory
 'Decision Tree for a search by either reqnum or job title, location and post date
 If Not reqnum = "" Then
 Dim reqnum_row As Long
 reqnum_row = 0
 'Search mechanism for requisition numbers
 For Each c In Sheets(1).Range("A:A")
 If c.Value = reqnum Then
 reqnum_row = c.Row
 End If
 Next c
 'if there isn't an exisiting req...
 If reqnum_row = 0 Then
 'Set a variable as the last row with text
 Dim rw As Long
 rw = LastRowInOneColumn("D", 1)
 'Establish the first empty row
 Dim newrow As Long
 newrow = rw + 1
 'Add information to row
 Call UpdateCellData(newrow)
 'Format the row with alternating background color
 Call FormatRows(newrow)
 Call CloseForm(newrow)
 Else
 Call UpdateCellData(reqnum_row)
 Call CloseForm(reqnum_row)
 End If
 Else
 'Logic for if there is not a reqnum
 Dim job_row As Long
 job_row = 0
 'Search mechanism for search by job title then location then post date.
 'If all given data equals all the same in a row it will be selected as the row to update
 For Each X In Sheets(1).Range("B:B")
 If X.Value = jbtitle Then
 If X.Offset(0, 1).Value = jbloc Then
 If X.Offset(0, 2).Value = postdate Then
 job_row = X.Row
 End If
 End If
 End If
 Next X
 'If the search comes up empty add a new row of data
 If job_row = 0 Then
 'Set a variable as last row with text
 Dim rwj As Long
 rwj = LastRowInOneColumn("D", 1)
 'set a variable as first row without text
 Dim newrowj As Long
 newrowj = rwj + 1
 Call UpdateCellData(newrowj)
 'Formatting the rows
 Call FormatRows(newrowj)
 Call CloseForm(newrowj)
 Else
 Call UpdateCellData(job_row)
 Call CloseForm(job_row)
 End If
 End If
End Sub

First off, see this?

 'Establish variables with textbox values
 reqnum = ReqNumBox.Value
 jbtitle = JobTitleBox.Value
 jbloc = JobLocationBox.Value
 postdate = DatePostedBox.Value
 closedate = DateClosedBox.Value
 jobcategory = CategoryComboBox.Value
 pidname = PIDComboBox.Value

This needs to be a Class Object. Let's create a Class called Job_Posting_Details or something similar and make all of those things Properties. Then we can just pass around a Job_Posting_Details Object instead of all of these variables.

Insert --> Class Module and then:

Option Explicit
Private Type JobPostingDetails
 RequisitionNumber As Long
 jobTitle As String
 jobLocation As String
 dateJobPosted As Date
 datePostingClosed As Date
 jobCategory As String
 pidName As String
End Type
Private this As JobPostingDetails
Public Property Let RequisitionNumber(ByVal inputValue As Long)
 this.RequisitionNumber = inputValue
End Property
Public Property Get RequisitionNumber() As Long
 RequisitionNumber = this.RequisitionNumber
End Property

With Property Let/Get for all the other properties:

Public Property Let JobTitle(ByVal inputValue As String)
 this.JobTitle = inputValue
End Property
Public Property Get JobTitle() As String
 JobTitle = this.JobTitle
End Property
Public Property Let JobLocation(ByVal inputValue As String)
 this.JobLocation = inputValue
End Property
Public Property Get JobLocation() As String
 JobLocation = this.JobLocation
End Property
Public Property Let DateJobPosted(ByVal inputValue As Date)
 this.DateJobPosted = inputValue
End Property
Public Property Get DateJobPosted() As Date
 DateJobPosted = this.DateJobPosted
End Property
Public Property Let DatePostingClosed(ByVal inputValue As Date)
 this.DatePostingClosed = inputValue
End Property
Public Property Get DatePostingClosed() As Date
 DatePostingClosed = this.DatePostingClosed
End Property
Public Property Let JobCategory(ByVal inputValue As String)
 this.JobCategory = inputValue
End Property
Public Property Get JobCategory() As String
 JobCategory = this.JobCategory
End Property
Public Property Let PidName(ByVal inputValue As String)
 this.PidName = inputValue
End Property
Public Property Get PidName() As String
 PidName = this.PidName
End Property

And now we can do things like this:

Dim jobDetails As Job_Posting_Details
Set jobDetails = New Job_Posting_Details
With jobDetails
 .RequisitionNumber = CLng(ReqNumBox.Text)
 .JobTitle = JobTitleBox.Text
 .JobLocation = JobLocationBox.Text
 .DateJobPosted = CDate(DatePostedBox.Text)
 .DatePostingClosed = CDate(DateClosedBox.Text)
 .JobCategory = CategoryComboBox.Text
 .PidName = PIDComboBox.Text
End With
AddToDirectory jobDetails
Public Sub AddToDirectory(Byref jobDetails As Job_Posting_Details)
 'Checks to see if the person selected "Other" as the value for either PID or Category
 If jobDetails.PidName = "Other" Then
 Dim newname As String
 newname = InputBox("Please add your name: ")
 'adds new name to directory
 With Worksheets("Hidden Data Storage")
 Dim lastRow As Long
 lastRow = LastRowInOneColumn("A", 2)
 Dim printRow As Long
 printRow = lastRow + 1
 .Cells(printRow, 1).Value = newname
 End With
 End If
 If jobDetails.JobCategory = "Other" Then
 Dim newcategory As String
 'asks for new category
 newcategory = InputBox("Please add the new category: ")
 'adds new category to the hidden directory
 With Worksheets("Hidden Data Storage")
 Dim lastRow As Long
 lastRow = LastRowInOneColumn("B", 2)
 Dim printRow As Long
 printRow = lastRow + 1
 .Cells(printRow, 2).Value = newcategory
 End With
 End If
End Sub

Or, you might even make AddToDirectory a method of jobDetails itself (In the class module):

Public Sub AddToDirectory()
 Const OTHER_PID_TEXT As String = "other"
 Const PID_NAME_COLUMN As Long = 1
 Const OTHER_CATEGORY_TEXT As String = "Other"
 Const JOB_CATEGORY_COLUMN As Long = 2
 Dim directorySheet As Worksheet
 Set directorySheet = ThisWorkbook.Sheets("Hidden Data Storage")
 Dim printRow As Long
 With directorySheet
 If this.PidName = OTHER_PID_TEXT Then
 printRow = .Cells(.Rows.Count, PID_NAME_COLUMN).End(xlUp).Row + 1
 .Cells(printRow, PID_NAME_COLUMN) = InputBox("Please add your name: ")
 End If
 If this.JobCategory = OTHER_CATEGORY_TEXT Then
 printRow = .Cells(.Rows.Count, JOB_CATEGORY_COLUMN).End(xlUp).Row + 1
 .Cells(printRow, JOB_CATEGORY_COLUMN) = InputBox("Please add the new category: ")
 End If
 End With
End Sub

And then your original sub goes:

 Dim jobDetails As Job_Posting_Details
 Set jobDetails = New Job_Posting_Details
 With jobDetails
 .RequisitionNumber = CLng(ReqNumBox.Text)
 .JobTitle = JobTitleBox.Text
 .JobLocation = JobLocationBox.Text
 .DateJobPosted = CDate(DatePostedBox.Text)
 .DatePostingClosed = CDate(DateClosedBox.Text)
 .JobCategory = CategoryComboBox.Text
 .PidName = PIDComboBox.Text
 End With
 jobDetails.AddToDirectory

Now this:

 'Check to make sure data in views and applications is a number
 If Not IsNumeric(ViewsBox.Value) Then
 MsgBox ("You put in a non-number for Views")
 Exit Sub
 Else
 vws = ViewsBox.Value
 End If
 If Not IsNumeric(ApplicantsBox.Value) Then
 MsgBox ("You put in a non-number for Applications")
 Exit Sub
 Else
 apps = ApplicantsBox.Value
 End If

Needs to be a separate validation function. Like so:

Public Function ValidateViewboxValue(ByVal viewBoxValue As Variant) As Boolean
 Dim isValid As Boolean
 isValid = IsNumeric(viewBoxValue)
 If Not isValid Then
 MsgBox ("You put in a non-number for Views")
 End If
 ValidateViewboxValue = isValid
End Function

And then:

 Dim viewBoxValue As Variant
 viewBoxValue = viewsbox.Value
 If Not ValidateViewboxValue(viewBoxValue) Then Exit Sub

And now, if our validation requirements ever change, we can go straight to ValidateViewboxValue without worrying about the rest of the code.

---

There's lots more I'd like to say, but this review is getting a bit long and there's plenty here to work with already. I especially recommend building a Class or 2 for your userform.

• Brad Johansen: Thank you for the review. That was really insightful, I'll have to learn more about classes. Also, for variable names, is it appropriate to use variables like z, w, x, etc. for the loops?
• Kaz: If your variables are literally no more meaningful than iterating over an array index, then x, y, z are conventional. But in most cases, even there, something like currentIndex or currentRow or somethingCounter would be better.
• Brad Johansen: Just noticed you used ReqNumBox.Text, and etc. for the other textboxes, instead of .Value, was I using .Value incorrectly or is there a benefit to .Text?
• Kaz: @RollTideBrad I prefer .Text because then I know the result is going to come out as a String. If I use .Value then it could get coerced to all sorts of things.