Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

In addition to what was already said, I'll add these points:

Public Sub Extend(ByVal sequence As Variant)
 Dim element As Variant
 For Each element In sequence
 collec.Add element
 Next element
End Sub

Here the only clue the calling code has about the expected type, is the parameter's name (sequence) - that reinforces what was already said about meaningful parameter names, but I find the way you've implemented this forces the client to add boilerplate code just to be able to pass values to this method.

A much more convenient (and self-documenting) way of taking in an arbitrary number of parameters, is to use a ParamArray, and iterate its indices instead:

Public Sub Extend(ParamArray items())
 Dim i As Integer
 For i LBound(items) To UBound(items)
 collec.Add items(i)
 Next
End Sub

That way the client code can do this:

Dim myList As New List
myList.Extend 12, 23, 34, 45, 56, 67, "bob", 98.99

Notice a VBA Collection doesn't enforce any kind of type safety, so the item at index 1 could be a String while the item at index 2 could be a Double, or even an Object of any given type.

I know nothing of , so maybe that isn't an issue, but if your list is meant to only contain one type of items, you have a problem here.

Speaking of which:

Public Property Get ToString() As String
 ToString = "[" & Join(seq.ToArray(collec), ", ") & "]"
End Property

I don't see seq.ToArray anywhere, but whatever it's doing, it's not accounting for the fact that your List can take Object items, and doing that would break your ToString implementation, while every other method would seemingly work as expected. You should verify whether an item IsObject before attempting to do something with it that wouldn't work on an object.

You might have seen this post this post, where I solved these issues in a .net-type List<T> (although my ToString doesn't list the contents).

In addition to what was already said, I'll add these points:

Public Sub Extend(ByVal sequence As Variant)
 Dim element As Variant
 For Each element In sequence
 collec.Add element
 Next element
End Sub

Here the only clue the calling code has about the expected type, is the parameter's name (sequence) - that reinforces what was already said about meaningful parameter names, but I find the way you've implemented this forces the client to add boilerplate code just to be able to pass values to this method.

A much more convenient (and self-documenting) way of taking in an arbitrary number of parameters, is to use a ParamArray, and iterate its indices instead:

Public Sub Extend(ParamArray items())
 Dim i As Integer
 For i LBound(items) To UBound(items)
 collec.Add items(i)
 Next
End Sub

That way the client code can do this:

Dim myList As New List
myList.Extend 12, 23, 34, 45, 56, 67, "bob", 98.99

Notice a VBA Collection doesn't enforce any kind of type safety, so the item at index 1 could be a String while the item at index 2 could be a Double, or even an Object of any given type.

I know nothing of , so maybe that isn't an issue, but if your list is meant to only contain one type of items, you have a problem here.

Speaking of which:

Public Property Get ToString() As String
 ToString = "[" & Join(seq.ToArray(collec), ", ") & "]"
End Property

I don't see seq.ToArray anywhere, but whatever it's doing, it's not accounting for the fact that your List can take Object items, and doing that would break your ToString implementation, while every other method would seemingly work as expected. You should verify whether an item IsObject before attempting to do something with it that wouldn't work on an object.

You might have seen this post, where I solved these issues in a .net-type List<T> (although my ToString doesn't list the contents).

In addition to what was already said, I'll add these points:

Public Sub Extend(ByVal sequence As Variant)
 Dim element As Variant
 For Each element In sequence
 collec.Add element
 Next element
End Sub

Here the only clue the calling code has about the expected type, is the parameter's name (sequence) - that reinforces what was already said about meaningful parameter names, but I find the way you've implemented this forces the client to add boilerplate code just to be able to pass values to this method.

A much more convenient (and self-documenting) way of taking in an arbitrary number of parameters, is to use a ParamArray, and iterate its indices instead:

Public Sub Extend(ParamArray items())
 Dim i As Integer
 For i LBound(items) To UBound(items)
 collec.Add items(i)
 Next
End Sub

That way the client code can do this:

Dim myList As New List
myList.Extend 12, 23, 34, 45, 56, 67, "bob", 98.99

Notice a VBA Collection doesn't enforce any kind of type safety, so the item at index 1 could be a String while the item at index 2 could be a Double, or even an Object of any given type.

I know nothing of , so maybe that isn't an issue, but if your list is meant to only contain one type of items, you have a problem here.

Speaking of which:

Public Property Get ToString() As String
 ToString = "[" & Join(seq.ToArray(collec), ", ") & "]"
End Property

I don't see seq.ToArray anywhere, but whatever it's doing, it's not accounting for the fact that your List can take Object items, and doing that would break your ToString implementation, while every other method would seemingly work as expected. You should verify whether an item IsObject before attempting to do something with it that wouldn't work on an object.

You might have seen this post, where I solved these issues in a .net-type List<T> (although my ToString doesn't list the contents).

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

In addition to what was already said, I'll add these points:

Public Sub Extend(ByVal sequence As Variant)
 Dim element As Variant
 For Each element In sequence
 collec.Add element
 Next element
End Sub

Here the only clue the calling code has about the expected type, is the parameter's name (sequence) - that reinforces what was already said about meaningful parameter names, but I find the way you've implemented this forces the client to add boilerplate code just to be able to pass values to this method.

A much more convenient (and self-documenting) way of taking in an arbitrary number of parameters, is to use a ParamArray, and iterate its indices instead:

Public Sub Extend(ParamArray items())
 Dim i As Integer
 For i LBound(items) To UBound(items)
 collec.Add items(i)
 Next
End Sub

That way the client code can do this:

Dim myList As New List
myList.Extend 12, 23, 34, 45, 56, 67, "bob", 98.99

Notice a VBA Collection doesn't enforce any kind of type safety, so the item at index 1 could be a String while the item at index 2 could be a Double, or even an Object of any given type.

I know nothing of , so maybe that isn't an issue, but if your list is meant to only contain one type of items, you have a problem here.

Speaking of which:

Public Property Get ToString() As String
 ToString = "[" & Join(seq.ToArray(collec), ", ") & "]"
End Property

I don't see seq.ToArray anywhere, but whatever it's doing, it's not accounting for the fact that your List can take Object items, and doing that would break your ToString implementation, while every other method would seemingly work as expected. You should verify whether an item IsObject before attempting to do something with it that wouldn't work on an object.

You might have seen this post, where I solved these issues in a .net-type List<T> (although my ToString doesn't list the contents).

lang-vb

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