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?

\$\endgroup\$
  • 2
    \$\begingroup\$ "Please only exit the Form via one of the buttons provided" - you could just hide the X on the form \$\endgroup\$ – user28366 Oct 5 '15 at 8:48
  • \$\begingroup\$ Oh, how does one do that? \$\endgroup\$ – Kaz Oct 5 '15 at 12:13
  • \$\begingroup\$ stackoverflow.com/questions/15153491/… \$\endgroup\$ – user28366 Oct 5 '15 at 12:34
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.

\$\endgroup\$
  • \$\begingroup\$ That would be because (at the time) AddItem hadn't been coded yet :) \$\endgroup\$ – Kaz Oct 13 '15 at 9:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.