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\$
  • 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\$ – RubberDuck Jul 21 '14 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\$ – RubberDuck Jul 21 '14 at 18:56
  • \$\begingroup\$ I dont need it, I just need the data. \$\endgroup\$ – lpau001 Jul 21 '14 at 19:05
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.
| improve this answer | |
\$\endgroup\$
  • \$\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\$ – lpau001 Jul 21 '14 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\$ – RubberDuck Jul 21 '14 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\$ – RubberDuck Jul 21 '14 at 20:02

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.