# Loop through different controls and enabled the state of the whole group VBA

I got the below code that is looping through all controls and changing the state in a after update event and enables new row for user to use. Everything works fine but i'm just wondering whether there is a better way to achieve the same result as what i have done is long winded. This will only enable the controls if the combobox has data if blank skip enable.

Private Sub Item1_AfterUpdate()
Call UpdateItemsToClean
End Sub

Private Sub UpdateItemsToClean()
Dim ctl As MSForms.Control
Dim lbl As MSForms.Label
Dim cmb As MSForms.ComboBox
Dim txtbox As MSForms.TextBox
Dim optbtn As MSForms.OptionButton
Dim i As Integer

For i = 2 To ItemsListFrame.Controls.Count
For Each ctl In ItemsListFrame.Controls
If TypeName(ctl) = "Label" Then
If ctl.Tag = "GroupItem" & i - 1 Then
Set lbl = ctl
If Controls("Item" & i - 1).Value <> vbNullString Then
Controls("OrderLbl" & i).Enabled = True
End If
End If

ElseIf TypeName(ctl) = "ComboBox" Then
If ctl.Tag = "GroupItem" & i - 1 Then
Set cmb = ctl
If Controls("Item" & i - 1).Value <> vbNullString Then
Controls("Item" & i).Enabled = True
End If
End If

ElseIf TypeName(ctl) = "TextBox" Then
If ctl.Tag = "GroupItem" & i - 1 Then
Set txtbox = ctl
If Controls("Item" & i - 1).Value <> vbNullString Then
Controls("Qty" & i).Enabled = True
Controls("UnitPrice" & i).Enabled = True
Controls("SubTotal" & i).Enabled = True
End If
End If
ElseIf TypeName(ctl) = "OptionButton" Then
If ctl.Tag = "GroupItem" & i - 1 Or ctl.Tag = "InOut" & i - 1 Then
Set optbtn = ctl
If Controls("Item" & i - 1).Value <> vbNullString Then
Controls("OptionIn" & i).Enabled = True
Controls("OptionOut" & i).Enabled = True
End If
End If
End If
Next ctl
Next i
End Sub

• I'd suggest to separate the "control cleaning" into different Subs for each control type, e.g. UpdateLabelsToClean and UpdateTextBoxesToClean. That way your overall method won't get too long and hard to follow and may help if one set of controls needs additional logic. – PeterT Oct 8 '18 at 13:46
• @PeterT Thank you for the input sounds like a good idea will do just that much appreciated your help. – QuickSilver Oct 8 '18 at 14:38

I rows are being added dynamically I would write a class to wrap each row of controls and respond to the combobox events.

Since there appears to be is a fixed number of rows, there is no reason to iterate over all the Controls in the userform.

These variables seem legacy code because they are set but never used.

Dim ctl As MSForms.Control
Dim lbl As MSForms.Label
Dim cmb As MSForms.ComboBox
Dim txtbox As MSForms.TextBox
Dim optbtn As MSForms.OptionButton


This code is repeated several times.

If Controls("Item" & i - 1).Value <> vbNullString Then
Controls("OrderLbl" & i).Enabled = True
End If


The If statements can usually be eliminated when setting Boolean values.

Controls("OrderLbl" & i).Enabled = Controls("Item" & i - 1).Value <> vbNullString


A helper variable would make the code even easier to read.

Enabled = Controls("Item" &  i - 1).Value <> vbNullString
Controls("Qty" & i).Enabled = Enabled
Controls("UnitPrice" & i).Enabled = Enabled
Controls("SubTotal" & i).Enabled = Enabled


## Refactored Code

Private Sub UpdateItemsToClean()
Const ItemCount As Long = 5
Dim Enabled As Boolean
Dim Index As Long
Enabled = True
For Index = 2 to ItemCount
If Enabled Then Enabled = Controls("Item" & Index - 1).Value <> vbNullString
Controls("OrderLbl" & Index).Enabled = Enabled
Controls("Item" & Index).Enabled = Enabled
Controls("Qty" & Index).Enabled = Enabled
Controls("UnitPrice" & Index).Enabled = Enabled
Controls("SubTotal" & Index).Enabled = Enabled