# Manipulating the global address book in Outlook

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

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

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

'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
keepLooping = True
End If

Next j

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

• See the remarks section here : msdn.microsoft.com/en-us/library/office/ff866704.aspx I dont think your code is the problem. – konijn Sep 6 '13 at 16:09
• Not going to help performance in any way, but I can't help mentioning that If (bool_expression) = true can (read: should) be rewritten as If (bool_expression) and If (bool_expression) = false can (read: should) be rewritten as If Not (bool_expression). – Mathieu Guindon Sep 11 '13 at 23:40
• @enderland hey, I just noticed, you're this guy on StackOverflow I answered an Outlook VBA question to and we had the exact same rep score! Hi there, welcome to CodeReview! – Mathieu Guindon Oct 30 '13 at 0:54
• @retailcoder indeed :) Not very many people who are comfortable or confident in Outlook VBA that's for sure. – enderland Oct 30 '13 at 1:57
• @enderland did you try the HierarchicalUser approach yet? – Mathieu Guindon Oct 31 '13 at 18:48

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
Next
Set Underlings = result
End Property

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 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

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 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.
• One thing though, if you have a very complex organization with dozens of hierarchy levels, there's a chance you run into a stack overflow error with this recursive approach. Not sure how deep the VBA call stack is allowed to get. – Mathieu Guindon Oct 30 '13 at 0:45
• Hah, your #3 nitpick is so true, but I've gotten into the habit of doing this because... unfortunately a lot of the VBA I write in my job may be looked at by non-programmers and sometimes the "if bool then" can be confusing. C'est la vie. – enderland Oct 30 '13 at 1:49
• I like the idea of making more functions for this... I've also avoided doing #1, because it's really easy to get caught up on weird things with it (see here for some discussion). Definitely torn on that one because of the extra typing :) I've gotten into the habit of initing my booleans even as false so it's more clear what's happening and why, somehwhat a personal preference I guess? – enderland Oct 30 '13 at 1:56
• This sort of approach would make it look a million times nicer though, that's for sure. – enderland Oct 30 '13 at 1:58
• Thanks - and a couple dozen times easier to follow, too! As for your comment just before this last one, I find it clutters up the code more than anything - it makes more to read and adds zero value. If non-programmers need to look at the code, then they need to learn the language. :) (I've never had any issues with inline initialization) – Mathieu Guindon Oct 30 '13 at 2:00

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


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
.....

• No infinite loop, just no iteration (vba's weird!) – Mathieu Guindon Oct 29 '13 at 20:48
• I don't think I would even want to write my code like that. I think I would still surround it with an If Statement. that seems like it could be a really bad habit to accidentally fall into. – Malachi Oct 29 '13 at 21:00
• Totally agree, except I wouldn't surround it with an If - the code is already nested deep enough as it is! Using For Each to iterate collections completely circumvents this problem. – Mathieu Guindon Oct 30 '13 at 1:06
• duh. why did I say that.... you are totally Right @retailcoder, and i am going to steal that comment. :) – Malachi Oct 30 '13 at 13:49