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.
for I = I to 42
? \$\endgroup\$