tags:

views:

117

answers:

3

I've got a f(x) that returns a collection of user controls. .Net lets me just treat the f(x) name as the collection. Is there something wrong with doing so?

ex)

Private Function GetCcB() As Collection(Of Reports_ucColumn)
    Dim cc As New Collection(Of Reports_ucColumn)
    cc.Add(Me.ucColumn0)
    cc.Add(Me.ucColumn1)
    cc.Add(Me.ucColumn2)
    Return cc
End Function

Then later on, all use the properties on the objects in the collection, like this:

    For i As Integer = 0 To csB.Count - 1
        If csB(i).Contains("Invalid") Or csB(i).Contains("Duplicate") Then
            GetCcB.Item(i).isValid = False
            GetCcB.Item(i).title = csB(i)
            If csB(i).Contains("Invalid") Then : GetCcB.Item(i).errCode = "005W"
            Else : GetCcB.Item(i).errCode = "005W" 'todo - chg this once we get a duplicate col err code
            End If
        Else
            GetCcB.Item(i).isValid = True
            GetCcB.Item(i).title = csB(i)
            GetCcB.Item(i).errCode = String.Empty
        End If
+1  A: 

Well, you keep creating new collections in memory each time you call GetCcB, so I would recommend using a variable instead. =)

Edit: In essence, the object (collection) that GetCcB returns is different (though not in content) each time you call it and takes up additional memory on the heap which has to be collected by the garbage collector when it makes a pass. Better to just grab the collection as a variable and use that.

J. Steen
Thanks. I figured it looked too neat and progammy to be a good choice.
aape
Pretty code definitely ain't always the best performing code. =)
J. Steen
+1  A: 

You'd want to use a singleton instead. It's a static class with a property that returns a private field of the class.

squillman
Very neat. Thanks for opening a new group of things for me to learn about.
aape
+1  A: 

By calling the method each time that you use the collection, what your code is effectively doing is:

For i As Integer = 0 To csB.Count - 1
   Dim cc As Collection(Of Reports_ucColumn)
   If csB(i).Contains("Invalid") Or csB(i).Contains("Duplicate") Then
      cc = New Collection(Of Reports_ucColumn)
      cc.Add(Me.ucColumn0)
      cc.Add(Me.ucColumn1)
      cc.Add(Me.ucColumn2)
      cc.Item(i).isValid = False
      cc = New Collection(Of Reports_ucColumn)
      cc.Add(Me.ucColumn0)
      cc.Add(Me.ucColumn1)
      cc.Add(Me.ucColumn2)
      cc.Item(i).title = csB(i)
      If csB(i).Contains("Invalid") Then
         cc = New Collection(Of Reports_ucColumn)
         cc.Add(Me.ucColumn0)
         cc.Add(Me.ucColumn1)
         cc.Add(Me.ucColumn2)
         cc.Item(i).errCode = "005W"
      Else
         cc = New Collection(Of Reports_ucColumn)
         cc.Add(Me.ucColumn0)
         cc.Add(Me.ucColumn1)
         cc.Add(Me.ucColumn2)
         cc.Item(i).errCode = "005W" 'todo - chg this once we get a duplicate col err code
      End If
   Else
      cc = New Collection(Of Reports_ucColumn)
      cc.Add(Me.ucColumn0)
      cc.Add(Me.ucColumn1)
      cc.Add(Me.ucColumn2)
      cc.Item(i).isValid = True
      cc = New Collection(Of Reports_ucColumn)
      cc.Add(Me.ucColumn0)
      cc.Add(Me.ucColumn1)
      cc.Add(Me.ucColumn2)
      cc.Item(i).title = csB(i)
      cc = New Collection(Of Reports_ucColumn)
      cc.Add(Me.ucColumn0)
      cc.Add(Me.ucColumn1)
      cc.Add(Me.ucColumn2)
      cc.Item(i).errCode = String.Empty
   End If
Next

You would never write code like that, would you?

You can turn the function into a property with lazy initialisation. Then it's ok to use it over and over, as the collection is only created once:

Private _cc As Collection(Of Reports_ucColumn)

Private Property CcB As Collection(Of Reports_ucColumn)
   Get
      If _cc Is Nothing Then
         _cc = New Collection(Of Reports_ucColumn)
         _cc.Add(Me.ucColumn0)
         _cc.Add(Me.ucColumn1)
         _cc.Add(Me.ucColumn2)
      End If
      Return _cc
   End Get
End Property

(Ideally the property should be public in a class and the _cc variable should be private, encapsulated in the class so that only the property could access it.)

Semantically this works fine also. A function generally does a bit of work, so you shouldn't call it over and over if you want to use the result over and over. A property on the other hand usually doesn't do much work, and if the result needs some processing to be created, the property usually does something like lazy initialisation or pre-initialisation to make sure that the work isn't done more than neccessary.

Guffa
Cool. Thank you much for the input and advice.
aape