0

I have written a macro that is supposed to open and close thousands of workbooks and take the information from these. It fills up the list in sheet2 and when it reaches row 50000 it calls a cleaning macro which sorts the data in sheet1. The macro seems to be working fine except for the memory consumption which keeps increasing until Excel tells me it has run out of it. I have tried implementing a workbook save every time the cleaner macro gets called as it seemed to have helped someone else on the forum with the same problem, but for me it did nothing. Does anyone have any ideas to fix this? I have included my code below.

Sub LoopAllExcelFilesInFolder()
'PURPOSE: To loop through all Excel files in a user specified folder

Dim wb As Workbook
Dim myPath As String
Dim myFile As String
Dim myExtension As String
Dim rc As Long
Dim wbRC As Long
Dim rs As Variant


On Error Resume Next


''Optimize Macro Speed
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

myPath = "C:\Users\QQQ\Documents\Macro testing\BoM_ALL\"
myExtension = "*.xlsx"

'Target Path with Ending Extention
myFile = Dir(myPath & myExtension)

'Loop through each Excel file in folder
Do While myFile <> ""
    If Worksheets(2).Range("A50000").Value <> "" Then
        Call Cleaner
    End If

    'Set variable equal to opened workbook
    Set wb = Workbooks.Open(Filename:=myPath & myFile)

    'DO stuff in opened wb ------------------------------------------------------------------
    wb.Worksheets(1).Activate
    Range("B:B,D:D,E:E").Select
    Selection.Delete Shift:=xlToLeft
    ActiveSheet.Range("a1").CurrentRegion.Select

    wbRC = Selection.Rows.Count
    rs = Application.Match(Range("C3").Value, ThisWorkbook.Worksheets(3).Range("A1:A66950"), 0)

    If Application.IsNumber(rs) Then
        Range("C2:C" & wbRC).Value = ThisWorkbook.Worksheets(3).Cells(rs, 2).Value

        Selection.AutoFilter
        ActiveSheet.Range("A1:C" & wbRC).AutoFilter Field:=2, Criteria1:=Array( _
            "1", "2", "3", "4", "5", "6", "A", "B"), Operator:=xlFilterValues
        Range("A1").Offset(1, 0).Select
        Range(Selection, Selection.End(xlToRight)).Select
        Range(Selection, Selection.End(xlDown)).Select


        Selection.Copy
        ThisWorkbook.Worksheets(2).Activate
        If Range("A1").Value = "" Then
            Range("A1").Select
        Else
            ActiveSheet.Range("a1").End(xlDown).Offset(1, 0).Select
        End If
        ActiveSheet.Paste
        ActiveSheet.Range(Selection.Address).RemoveDuplicates Columns:=Array(1), Header:=xlNo
    End If

    'Close Workbook
    wb.Application.CutCopyMode = False
    wb.Close SaveChanges:=False

    'Get next file name
    myFile = Dir
Loop
Call Cleaner
'Message Box when tasks are completed
MsgBox "Task Complete!"

'ResetSettings:
'Reset Macro Optimization Settings
Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True

End Sub



Sub Cleaner()

Dim rng1 As Range
Dim rng2 As Range
Dim cell As Range
Dim y As Variant
Dim ri As Long
Dim ci As Integer


Set rng1 = Worksheets(1).Range("A:A")
Set rng2 = Worksheets(2).Range("A:A")

Worksheets(1).Activate
ActiveSheet.Range("a1").CurrentRegion.Select
ri = Selection.Rows.Count
Range("A1").Select

For Each cell In rng2
    If cell.Value = "" Then
        ThisWorkbook.Worksheets(2).Activate
        ActiveSheet.Range("a1").CurrentRegion.Select
        Selection.Delete
        ThisWorkbook.Save
        Exit Sub
    End If
    'y = row location of match
    y = Application.Match(cell.Value, rng1, 0)
    'if not a match then write in the new machine number
    If Not Application.IsNumber(y) Then
        Cells(ri + 1, 1) = cell.Value
        Cells(ri + 1, 2) = cell.Offset(0, 2).Value
        ri = ri + 1
    'if mat number exists then write machine number in a new column
    Else
        ci = 2

        Do While True
            If Cells(y, ci).Value <> "" Then
                If Cells(y, ci).Value = cell.Offset(0, 2).Value Then
                    Exit Do
                End If
            Else
                Cells(y, ci) = cell.Offset(0, 2).Value
                Exit Do
            End If
            ci = ci + 1
        Loop
    End If
Next
ThisWorkbook.Worksheets(2).Activate
ActiveSheet.Range("a1").CurrentRegion.Select
Selection.Delete
ThisWorkbook.Save
End Sub
Goose
  • 11
  • 1
  • 2
  • 4
    Note (not the issue): Never use `On Error Resume Next` without error handing. This just hides the message but the errors still occur. So you are just blind and cannot see them. Remove it or implement an error handling instead or you can not debug/fix your code properly. Also read [How to avoid using Select in Excel VBA](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba) which makes your code much faster. – Pᴇʜ May 08 '18 at 08:36
  • 1
    This won't solve your problem, but try to [avoid using `Select`, `Activate` and `Paste`](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba?rq=1) where possible. – jsheeran May 08 '18 at 08:39
  • It's probably the copy & paste operation that uses up most of the memory. Try replacing this with Range("your destination range").Value = Range("your source range").Value – tsdn May 08 '18 at 09:08
  • 1
    Aside from the advice given above, as you are working with a big volume of files, try clearing your objects (i.e. `wb` object in `LoopAllExcelFilesInFolder` UDF). Objects do get cleared by garbage cleaner but if you do it as you go along, you can save some memory. Also, clear your clipboard after copy and paste, this will help with memory as well. Didn't read all of your code but this should help – Zac May 08 '18 at 10:03

3 Answers3

1

For your information about error handling (as @Pᴇʜ noted). Your On Error Resume Next swallows error in this line:

rs = Application.Match( _ Range("C3").Value, _ ThisWorkbook.Worksheets(3).Range("A1:A66950"), _ 0)

When value is not found, the error is thrown. Then you test whether rs contains error. But the problem is that for the reader of your code it's not clear that Match throws error when value is not found!

But my point is wider. Having top-level On Error Resume Next is dangerous because if error is thrown and then swallowed (like in case of rs), the program continues to execute in fault state!

Here's the illustration of my point.

Let's take two distinct actions:

  1. Searching for value with Match (as you do)
  2. Copying filtered range to some place.

All steps are described in comments.

You will be surprised that the filtered range won't be copied, but there will be message "No rows were filtered"!

Source range:

SO_Source_Range_Goose

VBA code:

Sub F()

    Dim rs As Variant
    Dim rng As Range
    Dim rngVisible As Range
    Dim rngData As Range

    '// Top-level error handling
    On Error Resume Next

    '// The whole table
    Set rng = Range("A1").CurrentRegion

    '// Table without a header
    With rng
        Set rngData = .Offset(1).Resize(.Rows.Count - 1)
    End With

    '// Search for non-existing value to generate error
    rs = WorksheetFunction.Match("PK", rng.Columns(1), 0)

    '// Filter by existing value
    rng.AutoFilter Field:=1, Criteria1:="DE"
    '// SpecialCells can throw error if there are no visible cells.
    '// Thus, we must check whether we have error!
    Set rngVisible = rngData.SpecialCells(xlCellTypeVisible)
    If Err = 0 Then
        '// Good - there are some visible rows
        rngVisible.Copy Range("E1")
    Else
        '// Message speaks for itself
        MsgBox "No rows were filtered!", vbCritical
    End If

    rng.Parent.AutoFilterMode = False

End Sub

The conclusion: set error handling as close as possible to the "dangerous" code.

Source workbook with code

JohnyL
  • 6,894
  • 3
  • 22
  • 41
0

I have not found any direct error in your code but I guess 50K calls may be to much for Excel VBA. I would try the following tricks:

  1. Try to avoid .Select. xlToRight and xlDown may be cheating, too. You can use e.g. Range(Cells(2, 1), Cells( Activesheet.usedrange.Rows, Activesheet.usedrange.columns)).Copy

  2. Put your loop - the code between do while and loop - into a Sub. VBA will release all vars and objects when exiting from the sub.

  3. Never believe in the success of an IO operation. Check Err.Number every time after IO op. E.g.

    Do While True
        Thisworkbook.Save
        If Err.Number = 0 then Exit Do
    Loop
    

    Apart from this, 50K opens and saves may result in a large amount of outstanding IO operations that may lead to crash.

EDITED ON PEH'S COMMENT

3.a A safer but more complex solution to manage excess IO and avoid endless loops:

Public Declare Sub Sleep Lib "kernel32" (ByVal dwMilliseconds as Long) 'For 32 Bit Systems
Dim iErr As Integer

For i=1 to 10
    On Error Resume Next        ' turn on rigth before an anticipated error
    Thisworkbook.Save
    If Err.Number = 0 then Exit For
    On Error Goto 0             ' turn off when you expect no more error
    Sleep (100)             ' wait 0.1 second
Next
If Err.Number <> 0 Then     ' if error after 10 tries then it must be something else
    iErr = Err.Number
    Err.Raise iErr
    ....

Of course this is a first shot, you should fine tune sleep time and number of times.

AcsErno
  • 1,597
  • 1
  • 7
  • 10
  • 2
    I don't conclude with your point 3. Having that loop will just run endless and hang your application (without letting the user know) if your file cannot be saved at all. That's a bad approach. Better to catch that error and throw a message or log it or something like that. Also if there was an error before the `Do` this will never exit even if save does not fail. – Pᴇʜ May 08 '18 at 09:22
  • @PEH: scholarly seeing you are absolutely right. This code in #3 is just a hint, as simple as possible. There are many ways to avoid endless loop that I did not want to detail. (BTW one who gets a task to process 50K files must be an experienced programmer who is not likely to fall in the trap of an endless loop.) The main message is below: 50K opens and closes in one loop can easily overload any computer and opsys that causes unpredictable behaviour. – AcsErno May 08 '18 at 11:23
  • 2
    Theoretically spoken I conclude with this *"one who gets a task to process 50K files must be an experienced programmer"* but practically this is by way not true. So many supervisors don't have a clue what they really ask their employees to do beyond their actual skills. And someone who is using `On Error Resume Next` in the very beginning for the whole procedure is *obviously* a not that much experienced programmer. Also keep in mind that further readers might trap into it. That's why I thought it was worth to mention it. • And yes 50k might be a too heavy load. – Pᴇʜ May 08 '18 at 11:44
  • 1
    Still have concerns with your updated code. Imagine `.Save` fails on `i=1` this loop will always run up to `i=10` even if save works on `i=2` because the error is not reset within the loop it cannot be `0` if it once raised. So if you want to do this then `On Error Resume Next` must be right before `.Save` and `On Error Goto 0` right after line `If Err.Number = 0 …` If you want to trigger a message it must be right before `On Error Goto 0` eg. an `Else` statement for `If Err.Number = 0 Then: Exit For: ElseIf Err.Number <> 0 And i = 10 Then: Err.Raise: End If` – Pᴇʜ May 09 '18 at 06:25
  • @Pᴇʜ _someone who is using `On Error Resume Next` in the very beginning for the whole procedure is **obviously** a not that much experienced programmer_ Ha! I know one experienced programmer who used to say: _The first line of all my procedures is `On Error Resume Next`_ :) – JohnyL May 10 '18 at 05:22
  • @PEH: Thank you for your comment. I was not entirely aware of how and when Err.Number is managed. Updated the code on your advice. – AcsErno May 10 '18 at 05:29
0

I once had the same problem where I had to update 1000nds of files and I ended up writing a wrapper-script (I would suggest to use VB-Script) to start Excel and trigger the macro to process a reasonable number of files. After that, Excel is completely closed and for the next iteration, a new Excel instance is opened.

In the macro, you have to keep track about which files you already processed, for example by writing a complete list of files into a sheet before starting the first iteration and write a flag like "processed" next to the file when it is handled.

This is a small example I tried. It is calling a function DoSomeOfTheWork that returns true if the work is completed (so that the VB-Script is not stuck in an endless loop). Script and Excel live in the same folder.

' Get Path of script
dim strPath, p
strPath = Wscript.ScriptFullName
p = inStrRev(Wscript.ScriptFullName, "\")
strPath = left(strPath, p)

' Loop until work is complete
dim allDone
allDone = false
do while not allDone
    dim objExcel, objWB
    Set objExcel = CreateObject("Excel.Application")
    set objWB = objExcel.Workbooks.Open(strPath & "\test1.xlsm")
    allDone = objWB.Application.Run("module1.DoSomeOfTheWork")
    ' For Debugging: MsgBox "Done? " & allDone
    objWB.close true
    objExcel.Application.Quit
    Set objExcel = Nothing
loop
FunThomas
  • 23,043
  • 3
  • 18
  • 34