How can I re-factor this code?

Question

I'm using the following function to map columns from a DataTable (passed from the data tier) to object properties. The function exists within the class I'm populating. The class has two methods: Load() which loads the object with values and LoadAll() which returns a collection of populated objects. I wanted to be able to use the same code to populate the current object or a new object. However I'm less than happy with the result, largely because of the duplication which is a bit of a maintenance nightmare.

Private Function MapDataRowToProperties(ByVal dr As DataRow, ByVal target As Incident) As Incident
            If target.Equals(Me) Then
               Me.ID = Convert.ToInt32(dr.Item("pkIncidentID"))
               Me.Description = dr.Item("IncidentDetail").ToString
               Me.Created = Convert.ToDateTime(dr.Item("CreatedOn"))
               ...
               Return Me
           Else
              Dim NewIncident As New Incident
              NewIncident.ID = Convert.ToInt32(dr.Item("pkIncidentID"))
              NewIncident.Description = dr.Item("IncidentDetail").ToString
              NewIncident.Created = Convert.ToDateTime(dr.Item("CreatedOn"))
              ...
              Return NewIncident
           End If

    End Function

Note: I'm well aware of ORM tools which will do this for me and I normally use EntitySpaces, but for this project I am unable to do so.

Answer 1

How about :

dim Inc as Incident 
if target.Equals(me) then 
   Inc = Me
else
   Inc = new Incident
end if 
'Other code'
return Inc

OTOH if target.Equals(me) is supposed to match only the current object (In that case you should use Is or ReferenceEquals as Equals can be overriden to return True for other objects) and you are passing a New Incident (or the target can be modified) in the second case then remove the if and use directly target.

Answer 2

Hi Simon,

Hope this helps. (Excuse me for the errors in syntax, it's long time since vb coding).

In the calling function

If target.Equals(Me) Then

    target = MapDataRosToProperties(target,dr)
Else

    Dim target as Incident

    target = MapDataRosToProperties(target,dr)

Refactored Function

Private Function MapDataRowToProperties(ByVal target as Incident,ByVal dr as DataRow) As Incident

    target .ID = Convert.ToInt32(dr.Item("pkIncidentID"))
    target .Description = dr.Item("IncidentDetail").ToString
    target .Created = Convert.ToDateTime(dr.Item("CreatedOn"))
    ...
    Return target 

End Function

Answer 3

It appears to me that your If and Else blocks only differ in the fact that one assumes an existing object(the current instance) has been passed and the other overwrites any passed reference by creating it anew.

This would seem like an ideal case for a ByRef parameter, in other words, let the calling code pass different variables and let this function work in the same way in each case :

Private Sub MapDataRowToProperties(ByVal dr As DataRow, ByRef target As Incident)
  If target Is Nothing Then
    target = New Incident()
  End If
  'Set properties 
  '...
End Sub

Sample invocation :

'Usage 1:
MapDataRowToProperties(dr, Me)

'Usage 2:
Dim inc as New Incident()
MapDataRowToProperties(dr, inc)

Answer 4

You've one function trying to do too more than one thing, seperate your intent and all will become clear

Public Class Incident

   Private sub CopyFrom(ByVal dr As DataRow) 
       MapDataRowToProperties(dr, me)
   End Function

   Private Shared Function CreateFrom(ByVal dr As DataRow) As Incident
      Dim NewIncident As New Incident
      MapDataRowToProperties(dr, me)
      Return NewIncident
   End Function

   Private Shared sub MapDataRowToProperties(ByVal dr As DataRow, byval target As Incident) 
      target.ID = Convert.ToInt32(dr.Item("pkIncidentID"))
      target.Description = dr.Item("IncidentDetail").ToString
      target.Created = Convert.ToDateTime(dr.Item("CreatedOn"))
   End Sub
End Class

Now usage becomes

   Dim aClass as new Incident
   aClass.CopyFrom(dr)

or

   Dim aClass as MyClass = Incident.CreateFrom(dr)