VBA userform loop to gather numbers from cells on an Excel sheet

Question

I am working on a userform and have everything working the way it should but it's taking a little longer than I would like. It's looping through 42 labels for each For statement and there are six of them.

In an effort to learn how to code a little more efficient, could someone please review my code and show me some faster ways to get the job done?

Private Sub Find()
Application.ScreenUpdating = False
Dim i As Integer, k As Integer
k = 1

i = 1

For i = i To 42
C1 = "C1_" & i

Me.Controls(C1) = Sheet14.Range("I" & k).Value

If Me.Controls(C1) = "0" Then
Me.Controls(C1).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C2 = "C2_" & i
Me.Controls(C2) = Sheet14.Range("J" & k).Value
If Me.Controls(C2) = "0" Then
Me.Controls(C2).ForeColor = &H8000000F

End If
k = k + 1

Next
'
k = 1

i = 1

For i = i To 42
C3 = "C3_" & i
Me.Controls(C3) = Sheet14.Range("K" & k).Value
If Me.Controls(C3) = "0" Then
Me.Controls(C3).ForeColor = &H8000000F

End If
k = k + 1

Next
k = 1

i = 1

For i = i To 42
C4 = "CL4_" & i
Me.Controls(C4) = Sheet14.Range("L" & k).Value
If Me.Controls(C4) = "0" Then
Me.Controls(C4).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C5 = "C5_" & i
Me.Controls(C5) = Sheet14.Range("M" & k).Value
If Me.Controls(C5) = "0" Then
Me.Controls(C5).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C6 = "C6_" & i
Me.Controls(C6) = Sheet14.Range("N" & k).Value
If Me.Controls(C6) = "0" Then
Me.Controls(C6).ForeColor = &H8000000F

End If
k = k + 1

Next
Application.ScreenUpdating = True
End Sub

Answer 1

i = 1 and For i = i To 42 is not how you initiate a loop.

Here is the Pseudo Code to write a For Next loop:

For i = Low_Bound to Upper_Bound

Next

This video will explain it better :Excel VBA Introduction Part 16 - For Next Loops. I recommend you watch the complete series on Youtube.

Here you set the ForeColor if one condition is met but you never reset it. Running the code twice will give not work properly.

If Me.Controls(C6) = "0" Then 
    Me.Controls(C6).ForeColor = &H8000000F
Else
    Me.Controls(C6).ForeColor = -2147483630
End If

&H8000000F is the hexadecimal code for the default ForeColor of a Userform. If you want to hide the label than change it's visibility.

Me.Controls(C6).Visible = Not Me.Controls(C6) = "0" 

Application.ScreenUpdating has no effect on a Userform. Use it when writing to or formatting a Range.

As a general rule of thumb, if you have a large amount of repeat code extract it to a helper function.

Refactored Code

Private Sub UpdateLabels()

    Dim Index As Long
    For Index = 1 To 42
        setLabel "C1_", "I", Index
        setLabel "C2_", "J", Index
        setLabel "C3_", "K", Index
        setLabel "CL4_", "L", Index
        setLabel "C5_", "M", Index
        setLabel "C6_", "N", Index
    Next

End Sub

Sub setLabel(Prefix As String, RefColumn As Variant, Index As Long)
    With Me.Controls(Prefix & Index)
        .Caption = sheet14.Cells(Index, RefColumn).Value
        If .Caption = "0" Then
            .ForeColor = &H8000000F
        Else
            .ForeColor = -2147483630
        End If
    End With
End Sub

Note: My goal was to make the process as simple as possible. I didn't bother loading the data into an Array because Read data from a Worksheet is an inexpensive process and we are only doing 252 lookups. The code ran virtually instantaneously on the test Userform.

Answer 2

What is your sub name? Find? That's a built in application function. And it doesn't explain at all what you're doing.

Also your indenting and spacing is way off base. Even if allowing for extra space, your function should look like this -

Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False
    Dim i As Integer, k As Integer
    
    k = 1
    i = 1

    For i = i To 42
        C1 = "C1_" & i
        Me.Controls(C1) = Sheet14.Range("I" & k).Value
        If Me.Controls(C1) = "0" Then
            Me.Controls(C1).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C2 = "C2_" & i
        Me.Controls(C2) = Sheet14.Range("J" & k).Value
        If Me.Controls(C2) = "0" Then
            Me.Controls(C2).ForeColor = &H8000000F
        End If
        k = k + 1
    Next
    
    k = 1
    i = 1

    For i = i To 42
        C3 = "C3_" & i
        Me.Controls(C3) = Sheet14.Range("K" & k).Value
        If Me.Controls(C3) = "0" Then
            Me.Controls(C3).ForeColor = &H8000000F
        End If
        k = k + 1
    Next
    
    k = 1
    i = 1

    For i = i To 42
        C4 = "CL4_" & i
        Me.Controls(C4) = Sheet14.Range("L" & k).Value
        If Me.Controls(C4) = "0" Then
            Me.Controls(C4).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C5 = "C5_" & i
        Me.Controls(C5) = Sheet14.Range("M" & k).Value
        If Me.Controls(C5) = "0" Then
            Me.Controls(C5).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C6 = "C6_" & i
        Me.Controls(C6) = Sheet14.Range("N" & k).Value
        If Me.Controls(C6) = "0" Then
            Me.Controls(C6).ForeColor = &H8000000F
        End If
        k = k + 1
    Next
    
    Application.ScreenUpdating = True
End Sub

I renamed it and added indents. Now lets move on to the code.

---

Variables

&H8000000F is a constant for vbButtonFace. I would at least give it a constant instead of repeating it over and over

 Const FACE_COLOR as Long = vbButtonFace

Your i and k variables are telling me nothing about what they are. Give me some context. Also, you haven't declared variables

C1
C2
C3
C4
C5
C6

What are those? Also, why are they being set to different strings?

C1 = "C1_" & i
C2 = "C2_" & i
C3 = "C3_" & i
C4 = "CL4_" & i 'Where did that L come from
C5 = "C5_" & i 'Where did that L go
C6 = "C6_" & i

I can't imagine what they are, or why they need to be set 42 times. Also it looks like you iterate the k the same number of times as the i, so why do you need the k?

---

Refactoring

You continually set i and k to 1, then iterate i from 1 to 42, increasing k by one each time. i and k are the same and they are always the same set of numbers.

Not only that, but you're doing the same exact thing 6 times -

For i = 1 To 42
    'something is something and i
    'something is Sheet14, letter and i
    'Check if it's 0 and change color
Next i

As far as I can tell, you change the Control string and the column letter. I'm sure we can create a procedure that takes arguments for this -

Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = sheet14.Cells(i, columnNumber)
        If Me.Controls(controlName) = 0 Then Me.Controls.ForeColor = vbButtonFace
    Next
End Sub

Now we have -

Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False

    Const CONTROL_STRING1 As String = "C1_"
    Const CONTROL_STRING2 As String = "C2_"
    Const CONTROL_STRING3 As String = "C3_"
    Const CONTROL_STRING4 As String = "CL4_"
    Const CONTROL_STRING5 As String = "C5_"
    Const CONTROL_STRING6 As String = "C6_"
    Const CONTROL_COLUMN1 As Long = 9
    Const CONTROL_COLUMN2 As Long = 10
    Const CONTROL_COLUMN3 As Long = 11
    Const CONTROL_COLUMN4 As Long = 12
    Const CONTROL_COLUMN5 As Long = 13
    Const CONTROL_COLUMN6 As Long = 14
    
    DoTheThing CONTROL_STRING1, CONTROL_COLUMN1
    DoTheThing CONTROL_STRING2, CONTROL_COLUMN2
    DoTheThing CONTROL_STRING3, CONTROL_COLUMN3
    DoTheThing CONTROL_STRING4, CONTROL_COLUMN4
    DoTheThing CONTROL_STRING5, CONTROL_COLUMN5
    DoTheThing CONTROL_STRING6, CONTROL_COLUMN6
    
    Application.ScreenUpdating = True
End Sub

Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = sheet14.Cells(i, columnNumber)
        If Me.Controls(controlName) = 0 Then Me.Controls.ForeColor = vbButtonFace
    Next
End Sub

I bet we could refactor it some more..

Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False
    Const BASE_STRING As String = "C"
    Const ALTERNATE_STRING As String = "CL"
    Const END_STRING As String = "_"
    Dim i As Long
    Dim controlString As String
    For i = 1 To 6
        If i = 4 Then
            controlString = ALTERNATE_STRING & i & END_STRING
        Else
            controlString = BASE_STRING & i & END_STRING
        End If
        DoTheThing controlString, i + 8
    Next
    Application.ScreenUpdating = True
End Sub

Now the whole thing is 27 lines. At first it was 95.

---

Performance

You said your loops are taking a long time. That's because you're accessing the sheet every time you loop. Bring those values into an array instead.

Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    Dim lookUpValues As Variant
    lookUpValues = GetArray(columnNumber)
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = lookUpValues(i,1)
        If Me.Controls(controlName) = 0 Then Me.Controls(controlName).ForeColor = vbButtonFace
    Next
End Sub

Private Function GetArray(ByVal columnNumber As Long) As Variant
    GetArray = Sheet14.Range(Sheet14.Cells(1, columnNumber), Sheet14.Cells(42, columnNumber))
End Function

---

Conclusion

So now instead of looping through the sheet constantly, you just get all the values you need into an array and loop through that. It's much faster than going to the sheet. We also eliminated 5 of the 6 loops by creating a procedure. It also appears much cleaner and less unwieldy.