1
\$\begingroup\$

My goal in this code review is to see if i can figure out how to optimize the Validate function takes up fewer lines of code. i know there must be a way to cut it down so that it doesnt span 300 lines of code.

The Validate function is designed to allow me to check all of the controls in an access form that have their tag field populated with the following tags. This list has grown over time, and the function was originally built a year and a half to two years ago. I figure i may as well get some opinions on this code before i go handing it to people like it is a good idea to use.

'INSTRUCTIONS
'set the tag field under the "other" tab NOTE: you can have multiple letters

  's - basic tag to indicate you want to save the value of the control
  'x - non-manditory fields that you still wish to save overrides f
  'r - specifically for radio buttons to parse the labels caption data
  'o - radio buttons where you want the value of the radio button instead of text, also 0 is not a valid number to set the radio buttons to
  'n - for float/double
  'd - indicates that the value must be a date
  'e - Read Only do not attempt to write to. This is more for loading data then saving data.
  'l - checks a listbox to see if it has any item data(Validation Only)
  'f - not manditory if control is disabled (ignore value if disabled)
  'g - modifier to f to check if it has a value and if it does save it
  'c - For AutoClearing
  'i - For Integer

They are actually a part of a larger set of instructions, but i wanted to focus on one function in that module at a time. (as much as i can considering helper functions.)

The style of label creation really depends on if you can just use the actual field name as your description of the field (which isn't always feasible) or the caption of the label that is associated with the field. ive found that the field isnt always bound to the controls object of whatever the field is, so i use the name of the field plus lbl tacked on the front. I also support the use of the default named labels, since sometimes its just quicker to let access generate a form for you and just adjust it.

Public Enum mscLabelDesc
    msclblstyle = -1 'Label name like lblParentControlName
    mscNone = 0 'No label used
    msc_labelStyle = 1 'Label name like ParentControlName_label
End Enum

This is the actual validation function. it takes the name of the form, one of the enumeration options, or if you want to use a different base validation tag that isnt "s", you can set that. I have had this happen exactly once so far, where i needed to validate two different sets of data. Unfortunately when i went through and upgraded this function to handle that situation, it turned into me just grabbing the function doubling the size.

Public Function Validate(ByVal argForm As String, Optional ByVal argLabelDesc As mscLabelDesc = mscNone, Optional ByVal argCustomTag As String = "") As String
    'if this ouputs an empty string, everything properly validated.
    'if argCustomTag is set then it will only validate controls that have that custom tag.
    If Not BasicInclude.DebugMode Then On Error GoTo Error_Handler Else On Error GoTo 0
    Dim ctrl As Control
    Dim out As String: out = vbCrLf
    Dim sourceForm As Form: Set sourceForm = Forms(argForm)
    If Len(argCustomTag) > 1 Then
        Err.Raise vbObjectError + 1, "Validate", "Custom tag can only be one character long"
    End If
    If argCustomTag = "" Then
        For Each ctrl In sourceForm.Controls
            If ctrl.Tag & "" <> "" Then
                If ctrl.Tag Like "*f*" And ctrl.Enabled Then
                    If ctrl.Tag Like "*n*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*i*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Or CStr(ctrl.value & "") Like "*.*" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*d*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsDate(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*l*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.ListCount = 0 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        ElseIf ctrl.ColumnHeads And ctrl.ListCount = 1 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*r*" Or ctrl.Tag Like "*o*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.value = 0 Then
                            ' if you want your radio button group to be not selected when you use this and catch the error, then set the default to 0 and start the valid value set at 1'
                            'considering changing this to check for -1 so a valid value set can be from 0 up.'
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*s*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Trim$(ctrl.value & "") = "" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    End If
                ElseIf Not ctrl.Tag Like "*f*" Then
                    If ctrl.Tag Like "*n*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*i*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Or CStr(ctrl.value & "") & "" Like "*.*" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*d*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsDate(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*l*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.ListCount = 0 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        ElseIf ctrl.ColumnHeads And ctrl.ListCount = 1 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*r*" Or ctrl.Tag Like "*o*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.value = 0 Then
                            ' if you want your radio button group to be not selected when you use this and catch the error, then set the default to 0 and start the valid value set at 1'
                            'considering changing this to check for -1 so a valid value set can be from 0 up.'
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*s*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Trim$(ctrl.value & "") = "" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    End If
                End If
            End If
        Next
    Else
        For Each ctrl In sourceForm.Controls
            If ctrl.Tag & "" Like "*" & argCustomTag & "*" Then
                If ctrl.Tag Like "*f*" And ctrl.Enabled Then
                    If ctrl.Tag Like "*n*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*i*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Or CStr(ctrl.value & "") Like "*.*" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*d*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsDate(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*l*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.ListCount = 0 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        ElseIf ctrl.ColumnHeads And ctrl.ListCount = 1 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*r*" Or ctrl.Tag Like "*o*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.value = 0 Then
                            ' if you want your radio button group to be not selected when you use this and catch the error, then set the default to 0 and start the valid value set at 1'
                            'considering changing this to check for -1 so a valid value set can be from 0 up.'
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*" & argCustomTag & "*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Trim$(ctrl.value & "") = "" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    End If
                ElseIf Not ctrl.Tag Like "*f*" Then
                    If ctrl.Tag Like "*n*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*i*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsNumeric(ctrl.value) Or CStr(ctrl.value & "") & "" Like "*.*" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*d*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Not IsDate(ctrl.value) Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*l*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.ListCount = 0 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        ElseIf ctrl.ColumnHeads And ctrl.ListCount = 1 Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*r*" Or ctrl.Tag Like "*o*" Then
                        If ctrl.Tag Like "*x*" Then
                        ElseIf ctrl.value = 0 Then
                            ' if you want your radio button group to be not selected when you use this and catch the error, then set the default to 0 and start the valid value set at 1'
                            'considering changing this to check for -1 so a valid value set can be from 0 up.'
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    ElseIf ctrl.Tag Like "*" & argCustomTag & "*" Then
                        If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
                        ElseIf Trim$(ctrl.value & "") = "" Then
                            If argLabelDesc = msclblstyle Then
                                out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
                            ElseIf argLabelDesc = msc_labelStyle Then
                                out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
                            Else
                                out = out & StripPrefix(ctrl.Name) & vbCrLf
                            End If
                        End If
                    End If
                End If
            End If
        Next
    End If
    out = Left$(out, Len(out) - 2)
Error_Exit:
    Validate = out
    Exit Function
Error_Handler:
    out = "The following error has occured" & vbCrLf & vbCrLf & _
          "Error Number: " & Err.Number & vbCrLf & _
          "Error Source: Validate" & vbCrLf & _
          "Error Description: " & Err.Description
    Resume Error_Exit
End Function

I only use a limited set of prefixs and then i have to deal with some of other peoples code that i update, sometimes very quickly.

Public Function StripPrefix(ByVal argIn As String) As String
    Dim PrefixList() As String: PrefixList = Split("txt,cmb,cbo,chk,lst,rad,opt", ",")
    Dim v As Variant
    For Each v In PrefixList
        If argIn Like v & "*" Then
            StripPrefix = Right$(argIn, Len(argIn) - Len(v))
            Exit Function
        End If
    Next
    StripPrefix = argIn
End Function

A simple example usage of the Validate function is as follows:

Public Sub Save()
        Dim Out as string
        Out = Validate(me.name,msclblstyle)
        If Out = vbnullstring Then
            'Do Save code here
        Else
            msgbox "Please check the following fields:" & vbCrLf & vbCrLf & out
        End If
End Sub
\$\endgroup\$
2
\$\begingroup\$

Your If-Then-Else-ElseIf-Then-ElseIf construct is really hard to follow. A complex If-Then construct should always be a red flag that there is a better way to do it.

Simplify the calling

When you want to use a form in your function, pass the form, not a string and hope that you can find the form again.

Public Sub Save()
        Dim Out as string
        Out = Validate(me,msclblstyle) ' Note slight change here.
        If Out = vbnullstring Then
            'Do Save code here
        Else
            msgbox "Please check the following fields:" & vbCrLf & vbCrLf & out
        End If
End Sub

--

Public Function Validate(ByVal sourceForm As Form, Optional ByVal argLabelDesc As mscLabelDesc = mscNone, Optional ByVal argCustomTag As String = "") As String
[...]
    ' Dim sourceForm As Form: Set sourceForm = Forms(argForm) <--- don't need this line any more.
[...]

Define your rules

When doing complex logic, define your rules up front so that they are easier to see and easier to maintain. You have a lot of repetition in your code and I suspect you cannot tell if you have actually repeated strains of logic.

An easy way is to identify "atoms" of logic and then build your "molecules" up.

Dim logicNotMandatory as Boolean
Dim logicFloatDouble as Boolean
Dim logicNonMandatoryStillSave as Boolean
[...]
logicNotMandatory = ctrl.Tag Like "*f*" 
logicFloatDouble = ctrl.Tag Like "*n*"
logicNonMandatoryStillSave = ctrl.Tag Like "*x*" 

Dim logicCheck1 ' I don't know your intent from your code - give this a meaningful name
Dim logicCheck2

logicCheck1 = logicNotMandatory And ctrl.Enabled
logicCheck2 = logicNonMandatoryStillSave  and Trim$(ctrl.value & "") = "" 
[...]

Yes, this will add lines to your code. But it will be much easier to debug and maintain. Especially if one of the rules changes slightly, and you only have to fix it in one place. It also means that the real work in the function is more readable and self-explanatory. I am sure that if you go through this exercise you will find many ways to refine your logic and your code base. The following is a direct translation from your original code (the first instance where the above logic is used):

        If logicCheck1 Then
            If logicFloatDouble Then
                If logicCheck2 Then

Consider setting flags and using Select Case

Identifying the above logic and identifying the limited number of DoAction outcomes means that you can set internal flags based on the logic.

Dim logicDoCheck1 as Boolean ' Again, find a meaningful name
Dim logicDoCheck2 as Boolean ' Again, find a meaningful name
logicDoCheck1 = logicCheck1 And logicFloatDouble and logicCheck2
logicDoCheck2 = [...]

There are two ways to approach the use of a flag variable. You could:

Dim myFlag as Long ' even better, set an enum
If logicDoCheck1 then myFlag = 1
If logicDoCheck2 then myFlag = 2 ' should be mutually exclusive. If not, revisit your logic.
[...]
Select Case myFlag
    Case 1
        ' Do something
[...]
End Select

Another trick here is to use the Select Case differently.

Select Case True
    Case logicDoCheck1
        ' Do Something
    [...]
End Select

Avoid ambiguity

What does the following even mean?

If ctrl.Tag Like "*x*" And Trim$(ctrl.value & "") = "" Then
ElseIf Trim$(ctrl.value & "") = "" Then

I suspect you really wanted

If Not (ctrl.Tag Like "*x*" Or Trim$(ctrl.value & "") = "" ) Then 
' Do things

If-Then statements are always easier to understand if they are a positive control, rather than a negative statement. Identify what it is you want when you want to do something, rather than try to identify some possible reasons why you do not want to do anything. It gets complicated even trying to explain it!

And then check for repetition

How often do you use this piece of code?

If argLabelDesc = msclblstyle Then
    out = out & sourceForm.Controls("lbl" & StripPrefix(ctrl.Name)).Caption & vbCrLf
ElseIf argLabelDesc = msc_labelStyle Then
    out = out & sourceForm.Controls(StripPrefix(ctrl.Name) & "_Label").Caption & vbCrLf
Else
    out = out & StripPrefix(ctrl.Name) & vbCrLf
End If

This looks like the key action , so using the Select Case method above will reduce your code base and complexity down a lot. Most of your logic is about deciding whether you should run the above bit of code.

You can deal with true repetition by creating a sub-routine or function with parameters to deal with the minor variations. However, in this case as there are no minor variations, I think the best way is to re-evaluate your logic so you only have one instance of this in your function.

What next?

If I was rewriting code for myself, I would stop reviewing here, totally rewrite the code based on the above and then re-start the review on the new code. What I have suggested above is a significant change to the code, but I hope this provided a lot of help and some new knowledge.

\$\endgroup\$

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.