Is there anything necessarily wrong with how I've done this?

[AUGuy]

I'm developing an idea I've got for a self-serve query system that my users can take advantage of. The goal is to eliminate some of the ad-hoc requests that I get and enable them to experiment with different datasets to find patterns, correlations, etc.

This is all based in a form and populated with VBA/SQL commands.

This particular code snippet handles the Criteria of the query they build (i.e. FIELD = String).

2=Enable Critiera. when criteria are enabled, it goes through and finds all the criteria combo boxes, enables them, and populates the value list with the set of fields the user has selected from a list box elsewhere on the form.

This code works perfectly, I'm just curious if I'm using best-practices here. Any feedback is welcome. If i'm not being clear let me know and I'll try to elaborate.

Private Sub ogButtonCriteria_Click()
Dim ctl As Control
Select Case ogButtonCriteria
    Case Is = 2
        If Me.hidCurFields.Value = "" Then
            MsgBox "You must add fields to the query before adding criteria!", vbExclamation, "Criteria Error"
            Me.ogButtonCriteria.Value = 1
            End
        Else
        For Each ctl In Me.Controls
            Select Case ctl.ControlType
                Case Is = acComboBox, acTextBox
                    If Left(ctl.NAME, 6) = "cbCrit" Or Left(ctl.NAME, 6) = "tbCrit" Then
                        ctl.Enabled = True
                        If ctl.ControlType = acComboBox Then
                            If Left(ctl.NAME, 10) = "cbCritTarg" Then ctl.Properties("RowSource") = Me.hidCurFields.Value
                        End If
                    End If
            End Select
        Next ctl
        End If
    Case Is = 1
         For Each ctl In Me.Controls
            Select Case ctl.ControlType
                Case Is = acComboBox, acTextBox
                    If Left(ctl.NAME, 6) = "cbCrit" Or Left(ctl.NAME, 6) = "tbCrit" Then
                        ctl.Enabled = False
                        If ctl.ControlType = acComboBox Then
                            If Left(ctl.NAME, 10) = "tbCritTarg" Then ctl.Properties("RowSource") = ""
                        End If
                    End If
            End Select
        Next ctl
    End Select
End Sub

 

[vbaInet]

It's not very clear what exactly you're doing without seeing the entire code and a screenshot of the form. However, you don't need to bother with all that because I'm sure it would be too long.

How many controls do you have that need enabling/disabling?

The other thing I see is repetition of code with respect to:

         For Each ctl In Me.Controls
            Select Case ctl.ControlType
                Case Is = acComboBox, acTextBox
                    If Left(ctl.NAME, 6) = "cbCrit" Or Left(ctl.NAME, 6) = "tbCrit" Then
                        ctl.Enabled = False
                        If ctl.ControlType = acComboBox Then
                            If Left(ctl.NAME, 10) = "tbCritTarg" Then ctl.Properties("RowSource") = ""
                        End If
                    End If
            End Select
        Next ctl

You can create a function for this that will accept two parameters, i.e. the Enabled property of the control and its Row Source. So call the function instead and pass those parameters to it.

Also, if all these controls are within a particular section, make reference to the section like this:

         For Each ctl In Me.[COLOR=Red]Detail[/COLOR].Controls

... where I'm making reference to the controls within Detail section of the form.