7
\$\begingroup\$

I was reading about similar problems to the one I am having, and my guess is that I am having a 'memory leak'. I'm not sure exactly what that means, or how to correct it. Could you take a look at my code and help me optimize? LastRow in this bit is a little over 70000.

start = Timer
For Row = 4 To LastRow
DoEvents
    If Original.Cells(Row, 4) <> "" Then
    Application.StatusBar = "Progress: " & Row & " out of " & LastRow & ": " & Format(Row / LastRow, "0.00%")
        'VLookUp method
'''''        Data.Cells(DataRow, 1).value = Original.Cells(Row, 4).value
'''''        Data.Cells(DataRow, 2).value = Original.Cells(Row, 39).value
'''''        Result = Evaluate("Vlookup('New Cost Data'!A" & DataRow & ",'PupFile Data'!B:D,3,false)")
'''''
'''''        If IsError(Result) = True Then
'''''            Data.Cells(DataRow, 3) = "No Old Cost"
'''''            DataRow = DataRow + 1
'''''        ElseIf Result = 0 Then
'''''            Data.Cells(DataRow, 3) = "No Old Cost"
'''''            DataRow = DataRow + 1
'''''        Else
'''''            Data.Cells(DataRow, 3) = Result
'''''            Data.Cells(DataRow, 4) = Format((Data.Cells(DataRow, 2) - Result) / Result, "0.00%")
'''''            DataRow = DataRow + 1
'''''        End If


    'Find() method
        Set RNGFound = Range(Pup.Cells(2, 2), Pup.Cells(Pup.Cells(Rows.count, 2).End(xlUp).Row, 2)).Find(Original.Cells(Row, 4))
        If Not RNGFound Is Nothing Then
           PupRow = Range(Pup.Cells(2, 2), Pup.Cells(Pup.Cells(Rows.count, 2).End(xlUp).Row, 2)).Find(Original.Cells(Row, 4), lookat:=xlWhole, searchorder:=xlRows, MatchCase:=True).Row
           Data.Cells(DataRow, 1).Value = Original.Cells(Row, 4).Value
           Data.Cells(DataRow, 2).Value = Original.Cells(Row, 39).Value
           Data.Cells(DataRow, 3).Value = Pup.Cells(PupRow, 4).Value
           Data.Cells(DataRow, 4) = (Data.Cells(DataRow, 2) - Data.Cells(DataRow, 3)) / Data.Cells(DataRow, 3)
        Else
            Data.Cells(DataRow, 1).Value = Original.Cells(Row, 4).Value
            Data.Cells(DataRow, 2).Value = Original.Cells(Row, 39).Value
            Data.Cells(DataRow, 3) = "No old Cost"
        End If
        DataRow = DataRow + 1
    End If
Next Row
Application.StatusBar = False
finish = Timer - start
MsgBox finish
Stop

The Vlookup() method took me about 500 seconds, but it slowed down considerably from the beginning. The find() method looked like it was taking much longer, so I am probably going with the vlookup, but what about the actual slowing down of the code? Is there something I need to change, or is slowing down over time just 'what happens'?

\$\endgroup\$
3
  • 2
    \$\begingroup\$ The vlookup is slowing down because Excel will recalculate the entire sheet each time the formula is entered into a new cell. Setting the calculation to xlManual prior to filling in the formulas should help. Just don't forget to turn it back to automatic after it's done, or it will never get calculated. \$\endgroup\$ Commented Jul 21, 2014 at 17:45
  • \$\begingroup\$ Scratch that^. You're using evaluate. Is there a particular reason you don't want to store the vlookup formula directly in the spreadsheet? \$\endgroup\$ Commented Jul 21, 2014 at 18:56
  • \$\begingroup\$ I dont need it, I just need the data. \$\endgroup\$ Commented Jul 21, 2014 at 19:05

1 Answer 1

Reset to default
6
\$\begingroup\$

I wish you would have posted more of your code here. I suspect that the code you posted should be broken out into it's own subroutine and we could have made sure you're using appropriate variable declarations. That being said, I'm only going to address your vlookup method, as you only need one or the other and I don't believe the find method is a good idea.

Per my comment, this code becomes slower because each time you add a formula via Data.Cells(DataRow, 4) = Format((Data.Cells(DataRow, 2) - Result) / Result, "0.00%"), Excel will recalculate all of the formulas in the spreadsheet. Prior to executing this code, you should set calculation to manual with Application.Calculation = xlCalculationManual.

If you don't have an error handler, you will need to add one so you can ensure that calculation is always set back to automatic with Application.Calculation = xlCalculationAutomatic. If you set it to manual, and then an error happens (without a handler), your workbook could cause you some confusion because it's not updating values when you think it should. You would also want to make sure to execute Application.StatusBar = False should any error happen. You don't want the status bar to get "stuck" if an error happens.

Some other things:

  • I'm not a fan of the PascalCase variable names. They should be camelCase to avoid confusion with class properties.
  • You don't need = True in IsError(Result) = True. It can simply be If IsError(result) Then.

  • That same If statement contains some duplication. It can be simplified as below.

    If (IsError(result)) Or (result = 0) Then
    Data.Cells(dataRow, 3) = "No Old Cost"
Else
    Data.Cells(dataRow, 3) = Result
    Data.Cells(dataRow, 4) = Format((Data.Cells(dataRow, 2) - Result) / Result, "0.00%")
End If

dataRow = dataRow + 1

  • Note that I also moved the variable incrementor outside of the If block. This also fixes what I believe is a bug that was causing you to skip rows. If you meant to skip rows, increment by two instead.

  • You're releasing the CPU to the OS at an inopportune time. Typically, DoEvents should come directly after you update the status bar. If you're concerned about saying it's at 100% while the code is still working, move the update to the end of the loop instead.
  • Unless you really feel like that status bar is needed, you should remove it. It's a performance hit as well. It sounds like you do need it though. Processing 70k rows takes some time and you don't want your users to think the program isn't responding.

The good stuff:

  • Thank you for ignoring Microsoft's recommendation to use hungarian notation!
  • Your variable names are clear and concise. No confusion about what is what. Well done.
\$\endgroup\$
3
  • \$\begingroup\$ I don't mind posting all of my code here. Should I edit my original question or add an answer with my complete code? Also, Thank you for your recommendations! I appreciate the feedback. \$\endgroup\$ Commented Jul 21, 2014 at 19:58
  • \$\begingroup\$ That's a bit of a touchy subject. We typically frown upon editing the code in the question once an answer is posted. It can invalidate answers. This meta post explains better than I can. It tackles the question of iterative review, but it generally applies I think. \$\endgroup\$ Commented Jul 21, 2014 at 20:01
  • 1
    \$\begingroup\$ Basically, no, but I wanted you to be aware of it for any future questions you ask. Welcome to Code Review! We need more vba questions and reviewers. I hope you stick around. \$\endgroup\$ Commented Jul 21, 2014 at 20:02

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.