14
\$\begingroup\$

I'm building a template [Process-Tracker] spreadsheet.

The idea is that, for any process where we can define what steps should occur in what order (and preferably, how far apart), there will be a spreadsheet with buttons for:

  • Adding a new [Item] to be tracked
  • Updating the List of [Items] and their progress
  • and a list of [Next Steps] ordered by [Due date].

I have written the [template] code for the Buttons and UserForm. The idea is that, when creating an actual process, I simply add specific validation for the expected variables/names and make sure it still outputs in a consistent format, and everything else will run just fine.

Here is my Form and My Code:

![enter image description here

[Worksheet] Add Item Button

Public Sub Button_AddItem_Click()
 CallFormInput
End Sub

Form Input Sub

Public Sub CallFormInput()
 UserFormButtonWasPressed = False
 UF_New_Process_Item.Show
 If Not UserFormButtonWasPressed Then PrintErrorMessage "Please only exit the Form via one of the buttons provided", showMessageBox:=True, endExecution:=True
End Sub

UserForm Code

Option Explicit
Option Compare Text
Private values As Collection
Private Sub UF_Exit_Form_Click()
 TestFormSyntax
 UserFormButtonWasPressed = True
 UF_New_Process_Item.Hide
 Exit Sub
End Sub
Private Sub UF_Add_Item_Click()
 Dim values As Collection
 Dim inputsWithFailedValidation As Variant
 inputsWithFailedValidation = Array()
 Dim validationMessage As String, passedValidation As Boolean
 Set values = New Collection
 UserFormButtonWasPressed = True
 ValidateAndAssignValues values, validationMessage, passedValidation, inputsWithFailedValidation
 If passedValidation Then
 AddItem values
 Else
 HandleFailedValidation validationMessage, passedValidation, inputsWithFailedValidation
 End If
End Sub
Private Sub UF_Add_Item_Recurring_Click()
 Dim values As Collection
 Dim inputsWithFailedValidation As Variant
 inputsWithFailedValidation = Array()
 Dim validationMessage As String, passedValidation As Boolean
 UserFormButtonWasPressed = True
 ValidateAndAssignValues values, validationMessage, passedValidation, inputsWithFailedValidation
 If passedValidation Then
 AddItem values
 Button_AddItem_Click '/ Starts again as if clicked the "Add Item" Button on the worksheet
 Else
 HandleFailedValidation validationMessage, passedValidation, inputsWithFailedValidation
 End If
End Sub
Private Sub ValidateAndAssignValues(ByRef values As Collection, ByRef validationMessage As Variant, ByRef passedValidation As Boolean, ByRef inputsWithFailedValidation As Variant)
 AssignValues values
 ValidateValues values, validationMessage, passedValidation, inputsWithFailedValidation
End Sub
Private Sub AssignValues(ByRef values As Collection)
 '/ Pass all values as text, formatting/validation will be handled later
 Dim i As Long
 Dim controlType As String
 Dim uf_Control As Control
 '/ item = control item text, key = control item name
 Set values = New Collection
 With Me.Controls
 For i = 0 To .Count - 1
 Set uf_Control = .Item(i)
 controlType = TypeName(uf_Control)
 If controlType = "textbox" Then
 values.Add uf_Control.Text, uf_Control.name
 End If
 Next i
 End With
End Sub
Private Sub ValidateValues(ByRef values As Collection, ByRef validationMessage As Variant, ByRef passedValidation As Boolean, ByRef inputsWithFailedValidation As Variant)
 '/ Convert to correct varType and validate - Names, Emails, Phone Numbers, Dates etc.
 '/ If failed validation, add to validation Message, add me.controls.item(key) to inputsWithFailedValidation
 '/ Set passed Validation True/False
 passedValidation = True
End Sub
Private Sub AddItem(ByRef values As Collection)
 UF_New_Process_Item.Hide
 Dim inputValues As Variant
 inputValues = Array()
 ReorderValuesCollection values, inputValues '/ Explicitly re-build values in a set order in a new array
 CreateNewProcessItem inputValues
End Sub
Private Sub HandleFailedValidation(ByRef validationMessage As Variant, ByRef passedValidation As Boolean, ByRef inputsWithFailedValidation As Variant)
 Dim uf_TextBox As TextBox
 PrintErrorMessage validationMessage, showMessageBox:=True, stopExecution:=False
 If ArrayIsAllocated(inputsWithFailedValidation) Then
 AssignArrayBounds inputsWithFailedValidation, LB1, UB1
 For ix = LB1 To UB1
 Set uf_TextBox = inputsWithFailedValidation(ix)
 uf_TextBox.Text = ""
 Next ix
 Else
 PrintErrorMessage "Validation failed but no objects specified", showMessageBox:=True, stopExecution:=True
 End If
End Sub
Private Sub TestFormSyntax()
End Sub

As well as general "Could I have written my code better" Feedback, I am particularly interested in how my code works as a template.

Could it be refactored/generalised even further?

Could I structure it to be even easier to modify?

Have I used a good (generic) naming convention re: distinguishing UserForm objects/subs from the rest of the program?

asked Sep 29, 2015 at 13:34
\$\endgroup\$
3
  • 2
    \$\begingroup\$ "Please only exit the Form via one of the buttons provided" - you could just hide the X on the form \$\endgroup\$ Commented Oct 5, 2015 at 8:48
  • \$\begingroup\$ Oh, how does one do that? \$\endgroup\$ Commented Oct 5, 2015 at 12:13
  • \$\begingroup\$ stackoverflow.com/questions/15153491/… \$\endgroup\$ Commented Oct 5, 2015 at 12:34

1 Answer 1

8
+150
\$\begingroup\$

You have added logic to an event of a button, which is not the best choice. You should extract most of the logic to at least separate methods or much better to a separate class.

First let us take a look at this

Private Sub ValidateValues(ByRef values As Collection, ByRef validationMessage As Variant, ByRef passedValidation As Boolean, ByRef inputsWithFailedValidation As Variant)
 '/ Convert to correct varType and validate - Names, Emails, Phone Numbers, Dates etc.
 '/ If failed validation, add to validation Message, add me.controls.item(key) to inputsWithFailedValidation
 '/ Set passed Validation True/False
 passedValidation = True
End Sub

I assume that you will implement proper validation instructions later on, but I wanted to point out that having a Sub instead of a Function which is taking a ByRef Boolean which is later queried isn't the way to go.

So let us change this to a function to get rid of that parameter.

Private Function ValidateValues(ByRef values As Collection, ByRef validationMessage As Variant, ByRef inputsWithFailedValidation As Variant) As Boolean
 ValidateValues = true
End Function 

The Sub AssignValues should be changed to a Function GetValues() As Collection because you have a Sub with only a ByRef parameter. As it seems that the form won't have more controls than 255 we can also change the Dim i As Long to Dim i As Integer.

Private Function GetValues() As Collection
 '/ Pass all values as text, formatting/validation will be handled later
 Dim i As Integer
 Dim controlType As String
 Dim uf_Control As Control
 Dim values As Collection
 '/ item = control item text, key = control item name
 Set values = New Collection
 For i = 0 To Me.Controls.Count - 1
 Set uf_Control = Me.Controls.Item(i)
 controlType = TypeName(uf_Control)
 If controlType = "textbox" Then
 values.Add uf_Control.Text, uf_Control.name
 End If
 Next i
 Set GetValues = values
End Function

Now let's switch to UF_Add_Item_Click() which is having almost identical code like the UF_Add_Item_Recurring_Click() method. This code duplication should be extracted to a separate method.

So let us create a method ProcessItem

Private Function ProcessItem() As Boolean
 ProcessItem = True
 Dim values As Collection
 Dim inputsWithFailedValidation As Variant
 inputsWithFailedValidation = Array()
 Dim validationMessage As String
 Dim passedValidation As Boolean
 UserFormButtonWasPressed = True
 Set values = GetValues()
 If Not ValidateValues(values, validationMessage, inputsWithFailedValidation) Then
 HandleFailedValidation validationMessage, passedValidation, inputsWithFailedValidation
 ProcessItem = False
 Else
 AddItem values
 End If
End Sub 

I have changed the style the local variables are defined, because having multiple variables defined on the same line should be avoided for readability.

I have added the call to AssignValues which helps to get rid of the ValidateAndAssignValues method.

Now the former UF_Add_Item_Click() and UF_Add_Item_Recurring_Click() method will look like so

Private Sub UF_Add_Item_Click()
 ProcessItem
End Sub
Private Sub UF_Add_Item_Recurring_Click()
 If ProcessItem() Then 
 Button_AddItem_Click '/ Starts again as if clicked the "Add Item" Button on the worksheet
 End If
End Sub

I can't tell much about the AddItem method because there are calls to methods which aren't in the posted code.

answered Oct 13, 2015 at 7:41
\$\endgroup\$
1
  • \$\begingroup\$ That would be because (at the time) AddItem hadn't been coded yet :) \$\endgroup\$ Commented Oct 13, 2015 at 9:11

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.