COM interop is in itself a performance hit, but even when accessing it through COM (e.g. in VBA), the Outlook type library / object model is notoriously, painfully slow. If performance is a requirement I would consider parsing the PST file without involving the Outlook object model, e.g. with something like PST-Parser (no affiliation, just an OSS project I found googling for PST parsers).
So, given a huge portion of the performance problem is primarily due to the approach (Outlook interop), I'm not going to review your code from a performance standpoint - but maintainability-wise, there are a number of issues.
Implicit access modifiers: Members in VB.NET are
Public by default, as they were in its VB6 ancestor. This is confusing, because in a lot of other programming languages, module members are
Private by default. In any case, avoid implicit access modifiers; specify them explicitly, leaving no room for ambiguity: if it's meant to be invoked from outside the module, make it
Public. If it's meant to be invoked from within the module, make it
Implicit ByVal modifiers: Parameters in VB.NET are passed by value unless specified otherwise. This is in stark contrast with VB6/VBA code, where parameters are passed by reference by default. A reader that's constantly context-switching between VBA and VB.NET code will have a much easier time if these modifiers are explicit everywhere - if you don't know what
ByRef means, I encourage you to lookup the difference: it's a rather important thing to know about.
Obsolete syntax: The explicit
Call syntax was already obsolete in VB6/VBA, over 17 years ago. There is no reason whatsoever to carry that keyword into your VB.NET code.
Inconsistent variable scopes: in VBA the smallest possible scope for a variable was member-level. In VB.NET blocks are also scopes, which means if you're declaring a local variable at procedure/member-level, and only using it inside a
For loop iteration, then the scope of that variable is wider than it needs to be, and that makes the code harder to follow. For example
InitIndexing really belong to a single iteration in that
For loop: them outliving the loop scope makes no sense. Declare variable as close as possible to their usage; don't have a "wall of declarations" at the top of a procedure.
A word about this:
Dim PropName, skey As String
In VBA that would declare
PropName as an implicit
skey as a
String. VB.NET "fixed" that, so both
skey are strings - but while it's perfectly legal to declare more than one variable with a single instruction, it's confusing and useless to do so, especially since
PropName appears to be constant and
skey in a wider scope than it needs to be - I'd much rather see this:
Const PropertyName As String = "http://schemas.microsoft.com/mapi/proptag/0x300B0102"
Avoid single-letter variable names - use meaningful, pronounceable names.
f is a folder - why not call it
skey looks like it's "key" with an "s-for-string" Systems Hungarian prefix that has absolutely zero value. What's that key representing? Looks like it's the string representation of a binary property. So, it's a property. How about
oPA is really
propertyAccessor, or perhaps
accessor is good enough in this context. That
o prefix reeks of System Hungarian ("o-for-object"? drop it! Everything is an object anyway!).
c means absolutely nothing. it's a
folderIndex, but you're iterating an object collection, and
doing that with a 1 isn't this much sexier?
For Each loop would be much faster than with a
For loop, which is faster for iterating arrays. Besides,
For Each subFolder In folder.Folder
r is again some seemingly randomly-picked single-letter name without a meaning. It stands for
path seems like a much better name. So that would be
Dim path As String = subFolder.FolderPath, but that seems like a superfluous allocation -
r is only used once, and conditionally at that; so you're unconditionally accessing a costly COM interop object to assign a value that you're only going to use if the key doesn't already exist in your dictionary - I'd move that variable to the conditional scope it belongs to.
ml leaves me wondering what it did to deserve a 2-letter name. You're iterating folder items, so
child seems a much better name to use.
The scope of
dict feels wrong too:
InitIndexing is really
GetAllFolderItems, and it should be a
Function that returns a dictionary rather than populating one that's outside its scope.
But then, when it's used, it's used as an
IEnumerable(Of KeyvaluePair) as the items are being iterated - you gain absolutely nothing by using a dictionary here, and the
Value of each item could very well be concatenated right there on-the-spot.
InitIndexing should be a
Function that returns an
IEnumerable(Of String), and with the awesomeness of
Yield Return you can even "yield" every item to the client loop and issue the results as they are being iterated:
Yield Return accessor.BinaryToString(accessor.GetProperty(PropertyName)) & ", " & subFolder.FolderPath
I'm not clear on exactly how it would be possible for the conditional to ever evaluate to
False (i.e. how could a key ever be duplicated?).
Yield Return you'd need to handle the recursive results a bit differently:
Dim subItem As String
For Each subItem In GetAllFolderItems(subFolder)
Yield Return subItem
And with that, the calling code can start enumerating the results well before every single item is known. Next step would be to collect these results asynchronously, but I'm not sure there'd be any real benefit to it (other than not freezing your app while it's collecting the data), since COM interop needs to marshal everything to one single thread anyway, and typically doesn't quite play well with concurrent or cross-thread accesses.
In any case, without
Yield Return you're iterating everything twice over; with it, you iterate the results once, as they're being returned - it should make a little bit of a difference.
1 That was true in VB6/VBA, but no longer with .NET.