Require advice on whether this is good practise or not.

  • Thread starter Thread starter Martin Horn
  • Start date Start date
M

Martin Horn

Hi group.

I have implemented a class along these lines,

Public Class CustomerDetails
Private fRecordString As String

Public Sub New(ByVal RecordString As String)
fRecordString = RecordString
End Sub

Public ReadOnly Property CustomerNumber() As String
Get
Dim Parts() As String
If fRecordString = "" Then Return ""
Parts = Split(fRecordString, vbCrLf)
Return Parts(C_NUMBER)
End Get
End Property

Public ReadOnly Property Name() As String
Get
Dim Parts() As String
Dim s As String
If fRecordString = "" Then Return ""
Parts = Split(fRecordString, vbCrLf)
If Parts(C_TITLE) <> "" Then s = Parts(C_TITLE) + " "
If Parts(C_FORENAME) <> "" Then s = s + Parts(C_FORENAME) + " "
If Parts(C_SURNAME) <> "" Then s = s + Parts(C_SURNAME)
Return s
End Get
End Property
End Class

I then use this code elsewhere in my program,

Dim en As IEnumerator = Records.GetEnumerator
While en.MoveNext
' Add each name to the Name list
lstNames.Items.Add(New CustomerDetails(en.Current.ToString).Name)
End While

This works exactly as I want, I used the parameterised constructer to pass
a string to the class, which then uses the name property to return a
formatted string all in one neat line of code.

The question I am (finally) asking is, is this creating a memory leak as I
am creating new instances of the class with no obvious way of disposing
of them, or will this be taken care of by the garbage collector? Also,
would this be classed as bad programming style.

As you can probably tell I am new to VB.NET having come from
a VB6 background and I haven't yet quite got my head around the
new programming features.

Thanks for your time,

Regards,

Martin Horn.
 
Martin,
The question I am (finally) asking is, is this creating a memory leak as I
am creating new instances of the class with no obvious way of disposing
of them, or will this be taken care of by the garbage collector?
Click to expand...
The GC will take care of the CustomerDetails objects as there are no
references to those objects.
Also, would this be classed as bad programming style.
Click to expand...
My only question would be, why do you have the code to split in multiple
places, potentially being called multiple times?

I would consider making Parts a private field of the class & split the input
string once in the constructor.
Public Class CustomerDetails
Private Parts() As String

Public Sub New(ByVal RecordString As String)
If fRecordString = "" Then Throw New ArgumentException("no input string", "RecordString")
Parts = Split(fRecordString, vbCrLf)
End Sub

Public ReadOnly Property CustomerNumber() As String
Get
Return Parts(C_NUMBER)
End Get
End Property

Public ReadOnly Property Name() As String
Get
Dim s As String
If Parts(C_TITLE) <> "" Then s = Parts(C_TITLE) + " "
If Parts(C_FORENAME) <> "" Then s = s + Parts(C_FORENAME) + " "
If Parts(C_SURNAME) <> "" Then s = s + Parts(C_SURNAME)
Return s
End Get
End Property
End Class
Click to expand...

Also rather have each property check for missing data, I simple do not allow
the creation of the object (by throwing an exception in the constructor). Of
course checking in each method is useful also, depending on the desired
effects...
Dim en As IEnumerator = Records.GetEnumerator
While en.MoveNext
' Add each name to the Name list
lstNames.Items.Add(New CustomerDetails(en.Current.ToString).Name)
End While
Click to expand...
Is there a reason you choose to use a while loop over a for each?

For Each record In Records
lstNames.Items.Add(New CustomerDetails(record).Name)
Click to expand...
Next

Presuming of course that Records is an array of String, if its something
else, I would pass the something else to the constructor of CustomerDetails.

Hope this helps
Jay
 
Jay,

Thanks for answering my question so quickly and helpfully and for confirming
what I hoped was the case regarding the GC.

My new class implementation,

Public Class CustomerDetails
Private Parts() As String

Public Sub New(ByVal RecordString As String)
If RecordString = "" Then Throw New ArgumentException _
("No Input String", "RecordString")
Parts = Split(RecordString, vbCrLf)
End Sub

Public ReadOnly Property CustomerNumber() As String
Get
Return Parts(C_NUMBER)
End Get
End Property

Public ReadOnly Property Name() As String
Get
Dim s As String
If Parts(C_TITLE) <> "" Then s = Parts(C_TITLE) + " "
If Parts(C_FORENAME) <> "" Then s = s + Parts(C_FORENAME) + " "
If Parts(C_SURNAME) <> "" Then s = s + Parts(C_SURNAME)
Return s
End Get
End Property

Rest of properties and methods...

End Class
Is there a reason you choose to use a while loop over a for each?
Click to expand...

Only that the book I have (Visual Basic .Net Unleashed) used IEnumerator as
a way of enumerating the strings in an array and it never occured to me
to try anything else.

My loop now look like this.
Dim s As String
For Each s In Records
' Add each name to the Name list
If s <> "" Then lstNames.Items.Add(New CustomerDetails(s).Name)
Next

Once again thanks very much for pointing me in the right direction.

Regards,

Martin Horn.
 
Back
Top