Extracting key information from a directory of XML files

Question

I have written some VBA to loop through a directory of 15,000 XML files and extract key information from these files into Excel - this is unbearably slow. Can someone please assist me in optimising the module?

Sub ImportXMLtoList()
 Application.DisplayAlerts = False
 Application.ScreenUpdating = False
 oldStatusBar = Application.DisplayStatusBar
 Application.DisplayStatusBar = True


 Dim myDir As String
 Dim StrFile As String
    StrFile = Dir("\\Ntsydfsp152\shared\Temp\Lucsan\LMIG\Output\XML\SBL\Staging\")
    myDir = "\\Ntsydfsp152\shared\Temp\Lucsan\LMIG\Output\XML\SBL\Staging\"
    myFile = "LMIG_List_XML_Files.xlsm"

 'clear sheet1
 Sheets(1).Range("a2:g30000").Clear
 x = 2

 Do While Len(StrFile) > 0

 Application.StatusBar = StrFile & " Row: " & x
    Sheets(1).Cells(x, 1).Value = StrFile
    Workbooks.OpenXML Filename:=myDir & StrFile, LoadOption:=xlXmlLoadImportToList
    temp2 = ActiveWorkbook.Sheets(1).Range("g2")
    temp3 = ActiveWorkbook.Sheets(1).Range("e2")
    temp4 = ActiveWorkbook.Sheets(1).Range("ak3")
    temp5 = ActiveWorkbook.Sheets(1).Range("av4") & "/" & ActiveWorkbook.Sheets(1).Range("ax4") & "/" & ActiveWorkbook.Sheets(1).Range("aw4") & "/" &     ActiveWorkbook.Sheets(1).Range("ay4")

    ActiveWorkbook.Close

    Sheets(1).Cells(x, 2) = temp2
    Sheets(1).Cells(x, 3) = temp3
    Sheets(1).Cells(x, 4) = temp4
    Sheets(1).Cells(x, 5) = temp5
    x = x + 1

    StrFile = Dir
 Loop

 Application.DisplayAlerts = True
 Application.ScreenUpdating = True
 Application.StatusBar = False
 Application.DisplayStatusBar = oldStatusBar

End Sub

Answer 1

A few short comments:

Use Option Explicit:

Why? because It makes thinks easier by orders of magnitude when developing.
You get more useful error messages and it becomes harder to misspell variable names without realizing.

That brings us right to the next:

Variable Names:

Your variable names are not consistently cased. You are mixing up "camel casing" (e.g. myDir) and "Pascal Casing" (e.g. StrFile)

Also temp 1 through 4 are not really good variable names. Neither is x, but hey ;)

Error Handling:

You should really make sure your "Application" is in a clean state after exit, even when you got unexpected errors:

Sub ImportXMLToList()
    On Error GoTo ErrorHandler

    ' Code goes here: 

:CleanExit
    Application.DisplayAlerts = True
    Application.ScreenUpdating = True
    Application.StatusBar = False
    Application.DisplayStatusBar = oldStatusBar

    Exit Sub
:ErrorHandler
    'Close resources, maybe errormessage
    Resume CleanExit
End Sub

Answer 2

I'll try not to duplicate advice, but seriously, use an error handler and make sure the application gets set back to a workable state. There's nothing more frustrating for a user than to end up with a "frozen" instance of Excel because screen updating has been turned off.

• I like that you're declaring variables for your directories and file names, but they should be constants instead. These are things that shouldn't change at runtime.

• This won't actually clear the sheet.

   'clear sheet1
   Sheets(1).Range("a2:g30000").Clear
  

  You should find the used range.

• The Sheets collection contains Charts and Pivot Tables. Use the Worksheets collection instead.

• x is a pretty bad variable name. row would be much more descriptive.

• You should declare a workbook variable instead of calling ActiveWorkbook all the time. It is possible for the user to select a different workbook while the code is running. Then the code is working on a different workbook and everything is a mess.

• Anytime you're numbering variables, you should step back and rethink the names. temp is a terrible name on it's own. Start numbering them and they're completely obfuscated.

• I recommend using named ranges instead of cell addresses. It's the difference between this:

  temp2 = ActiveWorkbook.Sheets(1).Range("g2")
  

  and this:

  personName = wb.Worksheets(1).Range("PersonName")
  

• You should initialize StrFile closer to the loop. It makes it clearer that you're looping through all files in a directory.

Finally...... Performance:

You can set the recalculation of the sheet to manual prior to running the code. It will only make a large difference if your code has a lot of formulas, but it's worth a shot. Just be sure to set it back to xlAutomatic in the "CleanExit" part of your routine.

Application.Calculation = xlCalculationManual

But really, never mind that because the real slow down here is opening and closing all of those workbooks. I recommend looking into connecting to those workbooks using ADODB recordsets. That should significantly speed up the process.

Answer 3

The other posts cover the main issues very well, so I won't belabour them and get right down to your performance issue. Your code is unbearably slow because using Excel to retrieve values from XML documents is using the bluntest of instruments available for the job. Kind of like chopping down a tree with a noodle. The network retrieval is not your issue at all - the bottleneck is how long it takes Excel to open a new Workbook. Just for a quick test, open an XML document in Excel, then open the same one in Notepad. Now imagine doing that 15,000 times. This is exactly what your code is doing, and is exactly why it is running slow.

Luckily, there are XML handling tools available in VBA - just add a reference to Microsoft XML and use them instead. I ran some tests to compare the 2 access methods, opening this file, retrieving a value, and writing it to a cell in the current Worksheet.

XML Method:

Sub FastRead(file As String, sheet As Worksheet)

    Dim current As DOMDocument
    Dim tree As IXMLDOMSelection
    Set current = New DOMDocument

    'For the sake of argument, we'll write the artist of the 4th CD to cell A1.
    If current.Load(file) Then
        Set tree = current.SelectNodes("CATALOG/CD")
        sheet.Cells(1, 1) = tree.item(3).ChildNodes(1).Text
    Else
        'Do whatever you need to do when you can't open the file.
        Debug.Print current.parseError.ErrorCode
    End If

End Sub

Excel Method:

Sub SlowRead(file As String, sheet As Worksheet)

    Dim worker As Workbook
    Set worker = Workbooks.OpenXML(file , , xlXmlLoadImportToList)

    'For the sake of argument, we'll write the artist of the 4th CD to cell A1.
    sheet.Cells(1, 1) = worker.Sheets(1).Cells(5, 2).Value

    Call worker.Close(False)

End Sub

Test Code

Sub Test()

    Dim sheet As Worksheet
    Set sheet = ActiveSheet

    Application.DisplayAlerts = False
    Application.ScreenUpdating = False

    Dim i As Integer
    For i = 1 To 10000
        Call FastRead("C:\cd_catalog.xml", sheet)
'        Call SlowRead("C:\cd_catalog.xml", sheet)
    Next i

    Application.DisplayAlerts = True
    Application.ScreenUpdating = True

End Sub

The FastRead version took 17 seconds over 10,000 calls. The SlowRead version clocked in at 28 minutes. Moral of the story is that Excel allows you to open XML files merely as a convenience - it is not intended to be an efficient way of retrieving values from them. Always best to use the right tools for the job.

Answer 4

On top of what the other people have said I can only recommend one more thing...

It seems you're accessing files on a network drive and doing that always adds overhead to the runtime.

Since you mention 15K files I would consider temporarily copying the directory that stores all those files to a local location on the drive and then running the code on the local copy of that directory instead of network.

You can then copy and paste +override the directory back to where it originally was.