AssignUnknown
isn't used anywhere, I'd remove it.Sort
,Min
andMax
don't make sense on aCollection
- it's permitted to compare apples with oranges and bananas. Sort / compare items of anInteger()
, aString()
, or aList
List
, but not of aCollection
.CloneCollection
should be calledToCollection
; its semantics are very similar to those ofToArray
, naming should be just as similar.- I'd remove the "Section" comments.
'Collection Wrappers
isn't useful. Neither is'Instance Methods
.'"Static" functions to be used with default instance of Enumerable
is a little better, but moot if the API gets fixed / split into static + instance API's. Merge
should be aSub
. It being aFunction
that doesn't return anything is quite confusing.- I like that
Merge
works with any array,Collection
, orList
List
(did you know that?). - I'd add a
Clear
method, to remove all items at once.
AssignUnknown
isn't used anywhere, I'd remove it.Sort
,Min
andMax
don't make sense on aCollection
- it's permitted to compare apples with oranges and bananas. Sort / compare items of anInteger()
, aString()
, or aList
, but not of aCollection
.CloneCollection
should be calledToCollection
; its semantics are very similar to those ofToArray
, naming should be just as similar.- I'd remove the "Section" comments.
'Collection Wrappers
isn't useful. Neither is'Instance Methods
.'"Static" functions to be used with default instance of Enumerable
is a little better, but moot if the API gets fixed / split into static + instance API's. Merge
should be aSub
. It being aFunction
that doesn't return anything is quite confusing.- I like that
Merge
works with any array,Collection
, orList
(did you know that?). - I'd add a
Clear
method, to remove all items at once.
AssignUnknown
isn't used anywhere, I'd remove it.Sort
,Min
andMax
don't make sense on aCollection
- it's permitted to compare apples with oranges and bananas. Sort / compare items of anInteger()
, aString()
, or aList
, but not of aCollection
.CloneCollection
should be calledToCollection
; its semantics are very similar to those ofToArray
, naming should be just as similar.- I'd remove the "Section" comments.
'Collection Wrappers
isn't useful. Neither is'Instance Methods
.'"Static" functions to be used with default instance of Enumerable
is a little better, but moot if the API gets fixed / split into static + instance API's. Merge
should be aSub
. It being aFunction
that doesn't return anything is quite confusing.- I like that
Merge
works with any array,Collection
, orList
(did you know that?). - I'd add a
Clear
method, to remove all items at once.
Clone
copies and returns the reference of the encapsulated collection, but CloneCollection
creates a new reference. The semantics of the two functions are very different, and bug-prone in the case of Clone
. Actually I'd get rid of it altogether. If client code needs the reference for the encapsulated collection, they can just take it.
...although I'd probably try to hide the encapsulated collection from the client code.
Clone
copies and returns the reference of the encapsulated collection, but CloneCollection
creates a new reference. The semantics of the two functions are very different, and bug-prone in the case of Clone
. Actually I'd get rid of it altogether. If client code needs the reference for the encapsulated collection, they can just take it.
...although I'd probably try to hide the encapsulated collection from the client code.
Design
I think you need to break that class in two. Having instance members on a static class is pretty confusing and bug-prone.
I'd suggest Enumerable
to be the static class, with these members (notice source
being preferred over collectionObject
, and explicit ByRef
modifiers and Variant
types):
Function Contains(ByRef source As Variant, ByVal value As Variant, Optional ByVal compareDefaultProperty As Boolean = False) As Boolean
Function First(ByRef source As Variant) As Variant
Function Last(ByRef source As Variant) As Variant
Function Intersect(ByRef source1 As Variant, ByRef source2 As Variant) As Iteratable
Function Distinct(ByRef source As Variant) As Iteratable
Function Clone(ByRef source As Variant) As Iteratable
Function ToArray(ByRef source As Variant) As Variant
Then, you can implement an Iteratable
class with a NewEnum
property; the beauty is that the instance variants of the static functions, can simply call on the static versions:
Public Function Contains(ByVal value as Variant, Optional ByVal compareDefaultProperty As Boolean = False) As Boolean
Contains = Enumerable.Contains(Me, value, compareDefaultProperty)
End Function
Public Function First() As Variant
First = Enumerable.First(Me)
End Function
Public Function Last() As Variant
Last = Enumerable.Last(Me)
End Function
And so on and so forth.
Potential Bugs
You're storing a Boolean
that "remembers" whether the encapsulated collection is sorted:
Private mCollection As Collection
Private mIsSorted As Boolean
The problem is that...
Public Property Set Collection(obj As Variant)
If TypeName(obj) = "Collection" Then
Set mCollection = obj
ElseIf TypeName(obj) = "Enumerable" Then
Set mCollection = obj.Collection
Else
Set mCollection = New Collection
Merge obj
End If
End Property
...the flag is going to lie whenever the Collection
property gets set. Simply setting mIsSorted = False
in the property setter fixes that.
...but it's more complicated than that:
Public Function Merge(collectionObject As Variant)
' Tries to convert any object passed in to a collection.
' This allows collection *like* objects such as Worksheets and Ranges.
On Error GoTo ErrHandler
Dim element As Variant
For Each element In collectionObject
mCollection.Add element
Next
'are we still sorted here?
and:
Public Sub Add(item, Optional Key, Optional Before, Optional After)
mCollection.Add item, Key, Before, After
'are we still sorted here?
End Sub
The static functions shouldn't tamper with the instance-level field:
Public Function Range(ByVal startValue As Long, ByVal endValue As Long) As Enumerable 'Collection
Set mCollection = New Collection
'...
Set Range.Collection = mCollection
That should really be a local Collection
reference.
Moreover, this code is legal:
Enumerable.Sort
Granted, it's a misuse of the class, but nothing forbids doing this:
Set Enumerable.Collection = New Collection
And abusing the default instance - that's where IsSorted
will tell the biggest lies, and break Min
and Max
implementations.
Splitting the class into a static Enumerable
class and a non-static Iteratable
class addresses this issue, since the static Enumerable
class has no reason to encapsulate an instance-level collection.
Range
should raise an error whenever startValue
is greater than endValue
.
Clone
copies and returns the reference of the encapsulated collection, but CloneCollection
creates a new reference. The semantics of the two functions are very different, and bug-prone in the case of Clone
. Actually I'd get rid of it altogether. If client code needs the reference for the encapsulated collection, they can just take it.
...although I'd probably try to hide the encapsulated collection from the client code.
Miscellaneous
AssignUnknown
isn't used anywhere, I'd remove it.Sort
,Min
andMax
don't make sense on aCollection
- it's permitted to compare apples with oranges and bananas. Sort / compare items of anInteger()
, aString()
, or aList
, but not of aCollection
.CloneCollection
should be calledToCollection
; its semantics are very similar to those ofToArray
, naming should be just as similar.- I'd remove the "Section" comments.
'Collection Wrappers
isn't useful. Neither is'Instance Methods
.'"Static" functions to be used with default instance of Enumerable
is a little better, but moot if the API gets fixed / split into static + instance API's. Merge
should be aSub
. It being aFunction
that doesn't return anything is quite confusing.- I like that
Merge
works with any array,Collection
, orList
(did you know that?). - I'd add a
Clear
method, to remove all items at once.