Skip to main content
Code Review

Return to Answer

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 List 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 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.
  • 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.5k
  • 18
  • 194
  • 467

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


Clonecopies 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.5k
  • 18
  • 194
  • 467

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

AltStyle によって変換されたページ (->オリジナル) /