Procedure names should always be verbs - they are things your program does, that's why. A better name might be IsInCollection - the only better alternative I can think of would be to implement your own collection/enumerable class implement your own collection/enumerable class, and make it have a Contains, or ContainsKey method... that might be overkill if this is the only time you're ever going to need to find a key in a collection. On the other hand, having a "toolbox" with frequently used class modules ready to import into any VBA project, can be handy.
Procedure names should always be verbs - they are things your program does, that's why. A better name might be IsInCollection - the only better alternative I can think of would be to implement your own collection/enumerable class, and make it have a Contains, or ContainsKey method... that might be overkill if this is the only time you're ever going to need to find a key in a collection. On the other hand, having a "toolbox" with frequently used class modules ready to import into any VBA project, can be handy.
Procedure names should always be verbs - they are things your program does, that's why. A better name might be IsInCollection - the only better alternative I can think of would be to implement your own collection/enumerable class, and make it have a Contains, or ContainsKey method... that might be overkill if this is the only time you're ever going to need to find a key in a collection. On the other hand, having a "toolbox" with frequently used class modules ready to import into any VBA project, can be handy.
Private Function IsInCollection(ByVal col As Collection, ByVal key As String) As Boolean
Dim result As Boolean
On Error GoTo CleanFail
'function logic goes here
CleanExit:
IsInCollection = result
Exit Function
CleanFail:
result = False
Resume CleanExit
End Function
Private Function IsInCollection(ByVal col As Collection, ByVal key As String) As Boolean
Dim result As Boolean
On Error GoTo CleanFail
'function logic goes here
CleanExit:
IsInCollection = result
Exit Function
CleanFail:
result = False
Resume CleanExit
End Function
Private Function IsInCollection(ByVal col As Collection, ByVal key As String) As Boolean
Dim result As Boolean
On Error GoTo CleanFail
'function logic goes here
CleanExit:
IsInCollection = result
Exit Function
CleanFail:
result = False
Resume CleanExit
End Function
@RubberDuck gave a pretty awesome review here, I'm just going to (削除) cover (削除ここまで) dissect the InCollection function, which was left out.
I like that you are sticking a comment at the top of every procedure - that's very good practice. One little nitpick about this one though:
'Returns boolean true if an object is within a collection
The value true is always going to be a Boolean value, and since the function's signature already specifies a Boolean return type, it's best to just leave it out. Also the comment is a little bit misleading - it doesn't reaturn true if an object is within a collection: it returns true when a specified key exists in a specified collection. Hence I'd rephrase it as such:
'Returns True when specified key exists in specified collection.
Public Function InCollection(col As Collection, key As String) As Boolean
Parameters are implicitly passed ByRef, which means the function is empowered with the ability to change the reference of col and the value of key before the execution flow returns to the caller: even if you're not doing that, it's best to pass parameters ByVal, and when you intend parameters to be passed by reference, to pass them explicitly ByRef.
Procedure names should always be verbs - they are things your program does, that's why. A better name might be IsInCollection - the only better alternative I can think of would be to implement your own collection/enumerable class, and make it have a Contains, or ContainsKey method... that might be overkill if this is the only time you're ever going to need to find a key in a collection. On the other hand, having a "toolbox" with frequently used class modules ready to import into any VBA project, can be handy.
But I digress. Let's dive into the procedure's body.
Dim var As Variant
I like that you're declaring the Variant type explicitly. As you may know, all VBA variables are of Variant type if you don't specify a type - being explicit is always a good thing.
The name isn't very good though: var is to Variant such as str is to String - and both are awful meaningless variable names. Naming things after their intent rather than their type usually helps with meaning. In this case value might be a better name.
Dim errNumber As Long
Don't need. I'll get back to that one.
InCollection = False Set var = Nothing
Don't need either. Any Boolean is False by default, so the function would return False if you never assigned its return value. It's good that you're explicitly assigning a value though, but assigning the return type's default value as the first executable line of code in the function feels wrong.
The type name of an unassigned Variant is Empty, and IsObject(var) would return False before that assignment. By setting it to Nothing, you have changed its type name to Nothing, and IsObject(var) would return True.
Since at this point we don't know yet exactly what we're going to get from the collection, we might as well leave it alone and let it be an Empty value.
Err.Clear
If executable code above that line raised an error, this line wouldn't execute, because execution would jump right out of the procedure since no On Error statement was encountered before. Hence, Err.Number can only be 0 at this point, and calling Err.Clear is useless. This line has no effect whatsoever, and can be removed.
On Error Resume Next var = col.Item(key) errNumber = CLng(Err.Number) On Error GoTo 0
Now you're treating var (which the VBA runtime now thinks is an Object) as a normal value type.
And here we are. I like that you are treating On Error Resume Next / On Error GoTo 0 as if it were a code block - that's pure awesomeness, because it makes it very clear what the "scope" of "resume next" is.
However On Error Resume Next / On Error GoTo 0 should only be used when you're expecting an error that you're ready to ignore. That isn't what's going on here: you are storing the error number in errNumber, to handle it later. That's poor error handling.
Err.Number is already a Long: the type conversion CLng is redundant, and can be safely removed.
Actually I would remove errNumber altogether. But I'll get back to that in a minute.
'5 is not in, 0 and 438 represent incollection If errNumber = 5 Then ' it is 5 if not in collection InCollection = False Else InCollection = True End If
This If...Else block clearly belongs in an error-handling subroutine*. You're handling error number 5 "Invalid procedure call or argument", and letting error 438 "Object doesn't support this property or method" mean yes, that key is in the collection. I think it's wrong.
Your function should only ever return True when it's absolutely 100% certain that the specified key was found.
You will encounter runtime error 450 "Wrong number of arguments or invalid property assignment" if the collection item with the specified key is actually an object reference - your code will correctly return True in that case, but in a somewhat awkward way.
In reality, the only error you should have to deal with is 5 "Invalid procedure call or argument" which means that the specified key isn't referring to anything in the collection. Any other error is due to code that isn't doing exactly what you're expecting it to be doing - and returning True when that happens is plain wrong.
So, how should it be handled then? First, make the very first executable line of code in the function be an On Error statement that will redirect execution flow to an error-handling subroutine:
Private Function IsInCollection(ByVal col As Collection, ByVal key As String) As Boolean
Dim result As Boolean
On Error GoTo CleanFail
'function logic goes here
CleanExit:
IsInCollection = result
Exit Function
CleanFail:
result = False
Resume CleanExit
End Function
The result variable is indeed superfluous, but it allows for a single spot to assign the function's return value. The CleanExit subroutine is responsible for assigning the function's return value, regarldess of the outcome - if the function runs into an error, it's still the exit point and only place where the function's return value is assigned. CleanFail only runs when any error occurs, in which case it explicitly sets the result to False. Simple, straightforward, foul-proof.
So how would the logic be implemented for this to work? We don't really care about the actual collection item - we only care whether it's there or not. Here's how I would do it:
result = TypeName(col(key)) <> vbNullString
Why TypeName? Because I know what type I'm getting: a String, regardless of whether the value is an Object or not. If the key exists, I'm getting the name of the type of the corresponding value. If it doesn't, runtime error 5 is handled in the CleanFail subroutine.
And since I don't care about the actual value, I'm not even allocating it a variable - either I get a non-null string (a null string is vbNullString - literally, it's a null string pointer; "" isn't a null string), or I get an error if and only if the key doesn't refer to an item in the collection.
Well, that ended up a longer answer than I anticipated. Good thing @RubberDuck covered pretty much everything else!
*@RubberDuck is misusing the term "subroutine" in his answer - he means "procedure". A subroutine is identified by a label, lives within a procedure and usually contains a returning mechanism, that the Return or Resume/Resume Next keywords provide. The VBA keywords for jumping to a subroutine are GoTo, and GoSub when you're planning on returning to the call site.