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

Question

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. enter image description here

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
 Controls("Comments" & 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

• PeterT: 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.
• QuickSilver: @PeterT Thank you for the input sounds like a good idea will do just that much appreciated your help.

Answer 1

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
Controls("Comments" & 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
 Controls("Comments" & Index).Enabled = Enabled
 Controls("OptionIn" & Index).Enabled = Enabled
 Controls("OptionOut" & Index).Enabled = Enabled
 Next
End Sub

• QuickSilver: You’re a genius never thought about it this way. You are right there about 13 rows of data at any given time. I can see from your code you are going in reverse from last row to 2nd and you are deducting 1. I was having an error because I couldn’t enable or disable the first or last row if it didn’t had any data in. Your code is much better than what I had I thank you for that. I guess I’m still a novice in programming 😥
• QuickSilver: Are you able to provide with an example of how you do a class to wrap each row and respond to combobox event? Probably that is a more advanced thing you can do but at the moment I’m not at that level yet.
• QuickSilver: I just realised from the code above and I think even mine we have same problem first row is enabled by default for my code and yours. Let’s say if all row are disable by default and enable just the first row when staff is selected you remove the -1 in the boolean and you what to loop including the first element 1? Also is there a reason you choose to do a reverse loop?? Why not a normal loop? Sorry for all the questions not in front of a computer to see how it runs.
• QuickSilver: I tried your way but even if the combobox has data in it still disables it and never set it back to true if you loop in reverse but then if you do normal loop it works. At the moment it seems your way is much cleaner than mine but it's not doing what i need it to do as there is still the issue that one of the rows will remain enabled or disabled depending how you loop top to bottom or the other way around.
• TinMan: If you can link me the file, I'll fix it.