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

• RubberDuck: Great advice, but it can be hard to control the case in VB because it's case insensitive and the IDE will automatically change it on you.
• user28366: Sorry Vogel but the OP says the code is slow and your tips don't really improve the execution time other than being just some nice cosmetic improvements.
• Vogel612: @vba4all I never claimed they'd improve the performance and frankly speaking, I am not knowledgeable enough to suggest performance-relevant changes :( But I think a clean and well defined Error Handling is more than a "cosmetic improvement" ... Either way why are you sorry?
• user28366: just don't want to you feel like im attacking you or something. just sharing my opinion

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.

• user28366: no offence but most of those tips don't really affect performance although they're good syntax related advices.
• RubberDuck: @vba4all you're absolutely right, but connecting to the sheets with ADODB will solve the performance problem. I felt there were other issues with the code. While I try to address any particular concerns, all of the code is up for review.
• RubberDuck: Well, at the least, it will speed it up. Processing 15k files is going to take some time regardless.

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.

• citizenkong: This is spot on. The Windows XML libraries make this so much easier - I've been working with them a lot recently in rebuilding a clunky Access application, and using MSXML and particularly using XPath expressions to pull data out has massively improved performance.
• user28366: somehow related

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.

• RubberDuck: I don't mean any offense, but it doesn't seem reasonable to copy 15k files twice.
• user28366: well, I would test it first :)
• RubberDuck: When you put it that way, it's worth trying. =;)-