Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  • AssignUnknown isn't used anywhere, I'd remove it.
  • Sort, Min and Max don't make sense on a Collection - it's permitted to compare apples with oranges and bananas. Sort / compare items of an Integer(), a String(), or a ListList, but not of a Collection.
  • CloneCollection should be called ToCollection; its semantics are very similar to those of ToArray, 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 a Sub. It being a Function that doesn't return anything is quite confusing.
  • I like that Merge works with any array, Collection, or ListList (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 and Max don't make sense on a Collection - it's permitted to compare apples with oranges and bananas. Sort / compare items of an Integer(), a String(), or a List, but not of a Collection.
  • CloneCollection should be called ToCollection; its semantics are very similar to those of ToArray, 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 a Sub. It being a Function that doesn't return anything is quite confusing.
  • I like that Merge works with any array, Collection, or 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 and Max don't make sense on a Collection - it's permitted to compare apples with oranges and bananas. Sort / compare items of an Integer(), a String(), or a List, but not of a Collection.
  • CloneCollection should be called ToCollection; its semantics are very similar to those of ToArray, 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 a Sub. It being a Function that doesn't return anything is quite confusing.
  • I like that Merge works with any array, Collection, or List (did you know that?).
  • I'd add a Clear method, to remove all items at once.
deleted 445 characters in body
Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 195
  • 469

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.


Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 195
  • 469

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 and Max don't make sense on a Collection - it's permitted to compare apples with oranges and bananas. Sort / compare items of an Integer(), a String(), or a List, but not of a Collection.
  • CloneCollection should be called ToCollection; its semantics are very similar to those of ToArray, 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 a Sub. It being a Function that doesn't return anything is quite confusing.
  • I like that Merge works with any array, Collection, or List (did you know that?).
  • I'd add a Clear method, to remove all items at once.