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.
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
private this As tHierarchicalUser
Public Property Get User() As ExchangeUser
Set User = this.User
Public Property Set User(value As ExchangeUser)
Set this.User = value
Public Property Get Superior() As HierarchicalUser
Set Superior = this.Superior
Public Property Set Superior(value As HierarchicalUser)
Set this.Superior = value
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
Set Underlings = result
Public Sub AddUnderling(underling As HierarchicalUser)
Set underling.Superior = Me
this.Underlings.Add underling 'you can use a key here to ensure uniqueness
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
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!
Set CreateHierarchicalUser = result
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
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)
Public Sub printAllReports(hierarchy As HierarchicalUser)
Dim reports As Collection
Set reports = hierarchy.FlattenHierarchy()
'do all that cool stuff you wanted to do!
- 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
=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