Manipulating the global address book in Outlook

Question

I am using VBA to manipulate the global address book in Outlook. My method takes a contact and returns a complete list of everyone who reports through that person based on the Outlook org structure.

Unfortunately it takes quite some time to run, even for a single manager. I'm not really sure what is the best way to improve the performance here - it sees the getDirectReports method takes some time, but, I don't see an easy way to determine if a user has reports prior to calling it first.

Public Sub printAllReports()

    Dim allReports As Collection
    Set allReports = New Collection

    Dim curLevelReports As Collection
    Set curLevelReports = New Collection

    Dim nextLevelReports As Collection
    Set nextLevelReports = New Collection

    Dim myTopLevelReport As ExchangeUser

    'this method returns an exchange user from their "outlook name"
    Set myTopLevelReport = getExchangeUserFromString("outlook resolvable name here")

    'add to both the next level of reports as well as all reports
    allReports.Add myTopLevelReport
    curLevelReports.Add myTopLevelReport

    Dim tempAddressEntries As AddressEntries
    Dim newExUser As ExchangeUser
    Dim i, j As Integer

    'flag for when another sublevel is found
    Dim keepLooping As Boolean
    keepLooping = False

    Dim requireValidUser As Boolean
    requireValidUser = False

    'this is where the fun begins
    Do

        'get current reports for the current level
        For i = curLevelReports.Count To 1 Step -1

            Set tempAddressEntries = curLevelReports.item(i).GetDirectReports

            'add all reports (note .Count returns 0 on an empty collection)
            For j = 1 To tempAddressEntries.Count
                Set newExUser = tempAddressEntries.item(j).getExchangeUser

                'isExchangeUserActualEmployee has some short boolean heuristics to make sure 
                'the user has at least a title and an email address
                If (isExchangeUserActualEmployee(newExUser) = True Or requireValidUser = False) Then
                    allReports.Add newExUser
                    nextLevelReports.Add newExUser
                    keepLooping = True
                End If

            Next j
            Set tempAddressEntries = Nothing


        Next i

        'reset for next iteration
        Set curLevelReports = nextLevelReports
        Set nextLevelReports = New Collection

        'no more levels to keep going
        If keepLooping = False Then
            Exit Do
        End If

        'reset flag for next iteration
        keepLooping = False

    Loop

    Dim oMail As Outlook.MailItem
    Set oMail = Application.CreateItem(olMailItem)


    'do stuff with this information (currently just write to new email, could do other cool stuff)
    For i = 1 To allReports.Count
        oMail.Body = oMail.Body + allReports.item(i).name + ";" + allReports.item(i).JobTitle
    Next i

    oMail.Display

End Sub

Answer 1

Your printAllReports method does almost everything that's possible to do with the Outlook API (ok maybe not), except printing anything. You basically have what we call a monolith, and that's bad because as your program changes, you are tempted to just keep adding and adding and adding, until the thing becomes an unmanageable, tangled mess. If you're going to call a method printAllReports, give it a signature like this: Sub printAllReports(allReports As Collection), so its intent is clear at first glance - and then make it do one thing; print all reports.

Performance-wise, the major hit is going to be hitting the Exchange server, so your code needs to make sure it hits the server only when it's necessary. If that's already the case, chances are you've already got it as good as it gets.

---

I think your multiple collections and 3-layer deep nested loop approach isn't the easiest way to make your code readable and maintainable, let alone to fine-tune its performance.

HierarchicalUser

The beautiful thing about a language that allows you to define objects, is that doing so actually adds vocabulary to that language, so no matter how lame VBA/VB6 is, with your own objects you can add new nouns, and with your own methods you can add new verbs, and with enough of them you actually end up crafting a language (ok, an API) that's beautiful in its own way.

You have the concept of an ExchangeUser, that can report to another ExchangeUser, and that can have ExchangeUser underlings. I call that a hierarchy, and I'd recommend you encapsulate that ExchangeUser into your very own HierarchicalUser class, something along those lines:

private type tHierarchicalUser
    User As ExchangeUser
    Superior As HierarchicalUser
    Underlings As New Collection
end type

private this As tHierarchicalUser
Option Explicit

Public Property Get User() As ExchangeUser
    Set User = this.User
End Property

Public Property Set User(value As ExchangeUser)
    Set this.User = value
End Property

Public Property Get Superior() As HierarchicalUser
    Set Superior = this.Superior
End Property

Public Property Set Superior(value As HierarchicalUser)
    Set this.Superior = value
End Property

Public Property Get Underlings As Collection
    'DO NOT return a reference to the encapsulated collection, you'll regret it!
    Dim result As New Collection, underling As HierarchicalUser
    For Each underling In this.Underlings
        result.Add underling
    Next
    Set Underlings = result
End Property

Public Sub AddUnderling(underling As HierarchicalUser)
    Set underling.Superior = Me
    this.Underlings.Add underling 'you can use a key here to ensure uniqueness
End Sub

'almost forgot!
Public Function FlattenHierarchy() As Collection
    Dim result As New Collection

    'traverse whole hierarchy and add all items to a collection that you return

    Set FlattenHierarchy = result
End Sub

Then you'll need a way to create instances of this class, and populate them. Enter the HierarchicalUserFactory (well, I know I would put that in its own class, but that's just me) - instead of nesting code we're going to be nesting method calls, recursively:

Public Function CreateHierarchicalUser(exUser As ExchangeUser) As HierarchicalUser
    Dim result As New HierarchicalUser
    Dim entry As AddressEntry
    Dim underling As ExchangeUser

    set result.User = exUser

    For Each entry In exUser.GetDirectReports() '<< For Each won't loop if there's nothing in the collection

        'if possible, run the isExchangeUserActualEmployee logic off this 'entry' object,
        'so you can only call the expensive GetExchangeUser method if needed:
        set underling = entry.GetExchangeUser
        result.AddUnderling CreateHierarchicalUser(underling) '<<< recursive call!

    Next

    Set CreateHierarchicalUser = result
End Function

Haven't tested any of this, but I believe an approach along those lines could possibly help you reduce the amount of GetExchangeUser calls and thus increase performance... not to mention readability++ :)

So your printAllReports method could possibly look like this now:

Public Function getHierarchy(topLevelUserName As String) As HierarchicalUser
    Dim factory As New HierarchicalUserFactory
    Dim topLevelUser As ExchangeUser

    Set topLevelUser = getExchangeUserFromString(topLevelUserName)
    Set GetHierarchy = factory.CreateHierarchicalUser(topLevelUser)
End Sub

Public Sub printAllReports(hierarchy As HierarchicalUser)

    Dim reports As Collection
    Set reports = hierarchy.FlattenHierarchy()

    'do all that cool stuff you wanted to do!

End Sub

---

Nitpicks

• When you declare an object variable and assign it to a New instance on the next line, consider combining the two statements into one: Dim X As New Y.
• When you declare a Boolean, it's automatically initialized to False so your post-declaration assignments are redundant.
• When you evaluate a Boolean expression in an If statement, you don't need to specify =True or =False - rather, just say If SomeBooleanExpression Then or If Not SomeBooleanExpression Then.
• When looping through objects in a collection, ALWAYS use a For Each construct. This will avoid weird stuff like For i = 1 To MyCollection.Count when .Count is 0, even someone that knows the VBA/VB6 collection base rules like the palm of their hand will go "huh?". For...Next has been around since before objects even existed, that construct is for traversing arrays. Collections deserve a For Each.

Answer 2

not sure that this will help with the Speed/Performance of the script but I am thinking that you should change your do while like this

Dim keepLooping as boolean 
keepLooping = True
Do While keepLooping = True
    keepLooping = False

and then the rest of the code the same. (except for changes to make it more efficient)

I also noticed this

        'add all reports (note .Count returns 0 on an empty collection)
        For j = 1 To tempAddressEntries.Count
            Set newExUser = tempAddressEntries.item(j).getExchangeUser

if there was an empty collection passed into this for some reason, wouldn't this cause an infinite loop?

you should instead write it with a Foreach

foreach item In temAddressEntries
    Set newExUser = item.getExchangeUser
.....