6
\$\begingroup\$

I have two datasets that are both within a worksheet, call them Data and IBES. The code checks whether the 6 variables are the same in each dataset and then writes the value from a specific column to the other dataset. To find this value the code runs through 288503 lines which is dramatically slow.

How can I speed up this code?

Public Function GetRightValue()

Dim i As Integer
Dim j As Long

Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False
Application.DisplayStatusBar = False
Application.EnableEvents = False
ActiveSheet.DisplayPageBreaks = False


For i = 2 To 1511           'Loop over all values from total dataset
        For j = 2 To 288503      'Loop over all values from IBES file
            If Worksheets("Data").Cells(i, 3) = Worksheets("IBES").Cells(j, 1) Then
                If Worksheets("Data").Cells(i, 7) = Worksheets("IBES").Cells(j, 6) Then
                    If Worksheets("Data").Cells(i, 10) = Worksheets("IBES").Cells(j, 9) Then
                        If Worksheets("Data").Cells(i, 13) = Worksheets("IBES").Cells(j, 11) Then
                            If Worksheets("Data").Cells(i, 8) = Worksheets("IBES").Cells(j, 7) Then
                                If Worksheets("Data").Cells(i, 14).Text = Worksheets("IBES").Cells(j, 13).Text Then
                                    Worksheets("Data").Cells(i, 12) = Worksheets("IBES").Cells(j, 10).Text
                                    Worksheets("Data").Cells(i, 18) = Worksheets("IBES").Cells(j, 16).Text
                                End If
                            End If
                        End If
                    End If
                End If
            End If
        Next j
Next i

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True
Application.DisplayStatusBar = True
Application.EnableEvents = True
ActiveSheet.DisplayPageBreaks = True

End Function
\$\endgroup\$
7
  • 2
    \$\begingroup\$ :) blog.codinghorror.com/flattening-arrow-code \$\endgroup\$ Commented Dec 12, 2015 at 17:46
  • \$\begingroup\$ If you can sort the datasets before you begin then you might reduce the number of checks significantly. Will each row in Data have a single match in IBES or could there be multiple IBES rows which match each Data row? \$\endgroup\$
    – barrowc
    Commented Dec 12, 2015 at 18:35
  • \$\begingroup\$ @Bjørn-RogerKringsjå how is this link helping? \$\endgroup\$ Commented Dec 12, 2015 at 18:51
  • \$\begingroup\$ @DirkReichel Well, I advice you to actually click the link and read the post.The context is highly relevant and you might learn something. After all, this is code review. \$\endgroup\$ Commented Dec 12, 2015 at 19:02
  • \$\begingroup\$ but the link isn't suitable for vba and this case :/ condition1 And condition2 in vba is slow cus condition2 wil also be calculated if the first one is false... at C condition1 && condition2 condition2 will not be calculated => in vba If condition1 Then: If condition2 Then is allways fater then If condition1 And condition2 Then... having no objects or anything like that, the link doesn't help for this case (or at least i can't find a useable improvement) :/ \$\endgroup\$ Commented Dec 12, 2015 at 19:11

2 Answers 2

8
\$\begingroup\$

Speed

When working with data at any kind of scale, the golden rule is Do not interact directly with the worksheet. Accessing worksheet objects has huge computational overhead, and you are accessing it every time you check a cell's contents.

Dirk's answer demonstrates how to put a range of values into an Array. Operating on an Array is typically 10-100x faster than operating on a worksheet. When you're finished, just set (Range) = (Array) to print your data back to your sheet.

Magic Variables

Not the good kind of magic either. A magic variable is any hard-coded value. E.G.

  For i = 2 To 1511           'Loop over all values from total dataset
    For j = 2 To 288503       'Loop over all values from IBES file

2 to 1511, 2 to 288503, Magic Variables. Where have those numbers come from? How do you know they're still up to date? I gather they're the final row(s) in your data series. Why not determine them dynamically?

Dim finalRow as long

finalRow = ws.Cells(Rows.Count, colNum).End(xlUp).Row

For i = 2 to finalRow '/ +1 to avoid Headers

    ...

Same goes for your column numbers. They should be named variables along the lines of condition1ColNum, condition2ColNum etc. Additionally, if you know what their headers are, I would start by iterating over your header row, and dynamically determining which columns they're actually in.

The key to all this is: How easy is it to break your Macro? If the answer is just "Add an extra row" or "Add/move a column" then it's not very good.

Also, you see this: Worksheets("Data").Cells(i, 3). You have that sheetName hardcoded 8 times. What if someone renames the sheet? Do you really want to go through and re-type it every time? Instead, use the VBa object model and create a proper Worksheet object:

Dim wsData as Worksheet, wsIbes as Worksheet

Set wsData = sheets("Data")
Set wsIbes = sheets("IBES")

Then you can use it like so:

wsData.Cells(i, 3)

And should the name change, you only have to change it in one place.


Even better, have you ever heard of CodeNames? In VBA, every worksheet object has a "Name" which is displayed in Excel and the user can rename at will unless it's protected. Each sheet also has a "CodeName", which can only be seen / modified in the VBE Window. Additionally, a sheet's CodeName actsas a Worksheet variable. E.G. If I have a sheet called "Data" which has CodeName "wsData" then I can simply write

wsData.Cells(i, 3)

Without having to declare anything. And now, if the user renames your sheet to "Something Something Data", your code won't break, because the codeName will remain unchanged.


For your main loop, you shouldn't be using nested If statements. It's messy and leads to Arrow Code. I would do it something like so:

For i = 2 To wsDataFinalRow 'Loop over all values from total dataset
    For j = 2 To wsIbesFinalRow 'Loop over all values from IBES file
        boolean1 = (Worksheets("Data").Cells(i, condition1DataColNum) = Worksheets("IBES").Cells(j, condition1IbesColNum))
        boolean2 = (Worksheets("Data").Cells(i, condition2DataColNum) = Worksheets("IBES").Cells(j, condition2IbesColNum))
        boolean3 = ...
        ...
        passedTest = boolean1 And boolean2 And boolean3 And ...

        If passedTest then
            ...
        End If

    next j
next i

And now it's much clearer what's going on, and it's much easier to add/move/re-arrange your test conditions at will, and you can add more actions based on a subset of conditions being true *without* having to write out a whole extra If ... If ... If ... If ... block.

\$\endgroup\$
9
  • \$\begingroup\$ Just looking for speed, nested If's is the best he can do... if the first test fails, why waste time to get everything else? Also I'm no friend of variables that will only be used once. IF doing all checks in one step, better use a line break ` _ ` and line up. Just for VBA someone told me some years ago: "ugly code runs best"... Still having an easy to understand code should come first, but if you need to choose which one would you pick: nice 2 minutes or ugly 20 seconds? (Still, nice fast code IS possible)... Only looking for the flattening arrow part in this comment :) \$\endgroup\$ Commented Dec 20, 2015 at 3:29
  • \$\begingroup\$ Using nested ifs or not isn't going to cause a dramatic difference in speed once they're using an Array. As to single-use variables. I take the line that "It takes very little effort to put them in variables" (and reduces the "Magic Strings" problem as well as introducing clear variable names), and it means if I *do* need to use them again in the future, it's already set up ready for me. \$\endgroup\$
    – Kaz
    Commented Dec 20, 2015 at 3:33
  • \$\begingroup\$ I'll never complain about the looks or anything like that. It is exactly what he should do. But running the code 1510 × 288502 times, it will be a difference if he does 8 checks or only ~3-5. However, having no data to check, he would need to test it for himself and then act just by having a bigger impact or not. At lest, when using the nested if's, he should check for best results and put them in the best order to change ~6-7 checks to ~2-3. Still, just when having a speed 》looks situation... \$\endgroup\$ Commented Dec 20, 2015 at 3:47
  • \$\begingroup\$ Perhaps, but it's also very inflexible. What happens if he wants to change his tests or add some new actions based on a different subset of results? \$\endgroup\$
    – Kaz
    Commented Dec 20, 2015 at 3:50
  • \$\begingroup\$ I'm of the view that you shouldn't prioritise speed optimisations until you know that you have a speed problem, and until he puts it in an array and runs it, we won't know. \$\endgroup\$
    – Kaz
    Commented Dec 20, 2015 at 3:51
4
\$\begingroup\$

Just looking at speed: Put all checks and outputs in to variables and then paste them in as less steps as possible like this:

Public Sub GetRightValue()

  Dim ibes_values As Variant
  Dim data_A_N_values As Variant
  Dim data_L_values As Variant
  Dim data_R_values As Variant

  ibes_values = Worksheets("IBES").Range("$A$1:$P$288503").Value
  data_A_N_values = Worksheets("Data").Range("$A$1:$N$1511").Value
  data_L_values = Worksheets("Data").Range("$L$1:$L$1511").Value
  data_R_values = Worksheets("Data").Range("$R$1:$R$1511").Value

  Application.EnableEvents = False
  Application.Calculation = xlCalculationManual

  Dim i As Long
  Dim j As Long

  For i = 2 To 1511           'Loop over all values from total dataset
    For j = 2 To 288503       'Loop over all values from IBES file
      If data_A_N_values(i, 3) = ibes_values(j, 1) Then
        If data_A_N_values(i, 7) = ibes_values(j, 6) Then
          If data_A_N_values(i, 10) = ibes_values(j, 9) Then
            If data_A_N_values(i, 13) = ibes_values(j, 11) Then
              If data_A_N_values(i, 8) = ibes_values(j, 7) Then
                If data_A_N_values(i, 14) = ibes_values(j, 13) Then
                  data_L_values(i, 12) = ibes_values(j, 10)
                  data_R_values(i, 1) = ibes_values(j, 16)
                  'Exit For 'would make sense to me
                End If
              End If
            End If
          End If
        End If
      End If
    Next j
  Next i

  Worksheets("Data").Range("$L$1:$L$1511") = data_L_values
  Worksheets("Data").Range("$R$1:$R$1511") = data_R_values

  Application.EnableEvents = True
  Application.Calculation = xlCalculationAutomatic

End Sub

Still one question: why is it a function?

EDIT
to show the reason for that try this 2 sub's:

Sub test1()
  Dim time_val As Double
  Dim i As Long, j As Long
  time_val = CDbl(Now)
  For i = 1 To 1000
    For j = 1 To Range("1:1").Count
      If Cells(i, j).Value = 1 Then
      End If
    Next
    DoEvents
  Next
  Debug.Print CDate(CDbl(Now) - time_val)
End Sub

Sub test2()
  Dim time_val As Double
  Dim loop_range As Variant
  Dim i As Long, j As Long
  time_val = CDbl(Now)
  loop_range = Range("1:1000").Value
  For i = 1 To 1000
    For j = 1 To Range("1:1").Count
      If loop_range(i, j) = 1 Then
      End If
    Next
    DoEvents
  Next
  Debug.Print CDate(CDbl(Now) - time_val)
End Sub

Then simply compare the time each of them took (for doing nothing, the first test needs a LOT of time)
For me Test1 took 71 seconds but test2 only 4 :)

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Hi Dirk, Thanks for your reply ! However, i do not understand why your code would be faster? \$\endgroup\$
    – F1990
    Commented Dec 12, 2015 at 18:49
  • 1
    \$\begingroup\$ @F1990 Reading from and writing into values is a LOT faster than interacting with sheets... => asking for speed = put everything into variables and paste them afterwars \$\endgroup\$ Commented Dec 12, 2015 at 18:55
  • \$\begingroup\$ The idea is that VBA is powerful for putting sheets in tables, and tables in sheets, powerful for working with tables, and weak for working with sheet data in great numbers. So, you lose very few execution time by making the home & away move from sheet to table, and win a lot of time by making all operations within the tables. Good job, Dirk. \$\endgroup\$
    – gazzz0x2z
    Commented Dec 12, 2015 at 19:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.