1
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – PeterT Oct 8 '18 at 13:46
  • \$\begingroup\$ @PeterT Thank you for the input sounds like a good idea will do just that much appreciated your help. \$\endgroup\$ – QuickSilver Oct 8 '18 at 14:38
2
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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 😥 \$\endgroup\$ – QuickSilver Oct 9 '18 at 12:20
  • \$\begingroup\$ 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. \$\endgroup\$ – QuickSilver Oct 9 '18 at 12:35
  • \$\begingroup\$ 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. \$\endgroup\$ – QuickSilver Oct 9 '18 at 14:37
  • \$\begingroup\$ 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. \$\endgroup\$ – QuickSilver Oct 9 '18 at 21:44
  • \$\begingroup\$ If you can link me the file, I'll fix it. \$\endgroup\$ – TinMan Oct 9 '18 at 21:55

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.