Long list of SQL parameters and audits

Question

Say I have the following code. What are your thoughts on questions 1-4 below?

1. I think it's way too long when I have to list out most of the fields. Is there any better way of doing this?

2. I build up the DB column name so it can be used later in my select and insert statement.

3. This is where and how I use the string I build up in Section 2. Does it look good or ugly?

4. Audit is required after inserting/update a new record, therefore I need to get the old and new value. There should be a better way to do this I guess, but I just have no idea how.

I'm open to any suggestion that can make this code block short and more readable.

Public Function InsertOrUpdateData(ByVal patient As Patient, ByVal sqlMode As SQLMode, ByVal affectedColumn As String, Optional ByVal updateString As String = "") As CreatePatientStatus
    Using conn As New SqlConnection(_myConnectionString)
        Dim cmd As New SqlCommand
        cmd.Connection = conn
        conn.Open()
        Try
            'SECTION 1
            'first I populate all the param before perform any action
            With cmd.Parameters
                .AddWithValue("@PCNO", patient.PCNO)
                .AddWithValue("@PName", patient.PName)
                .AddWithValue("@NRICType", patient.NRICType)
                .AddWithValue("@NRIC", patient.NRIC)
                'Another 80+ fields
                .AddWithValue("@Created_Date", Now)
                .AddWithValue("@Created_By", patient.CreateOrEditBy)
                .AddWithValue("@Edit_Date", Now)
                .AddWithValue("@Edit_By", patient.CreateOrEditBy)
                .AddWithValue("@FirstSeen", Now)
            End With

            'SECTION 2
            'this is list out all the affected DB column (It's FIXED, and always be)
            Dim sbAffectedDBFields As New StringBuilder
            sbAffectedDBFields.Append("PCNO,PName,NRICType,NRIC,")
            sbAffectedDBFields.Append("Anoter 80+ fields")
            sbAffectedDBFields.Append("Created_Date,Created_By,Edit_Date,Edit_By,FirstSeen")

            'oldValue and newValue to be used later part for audit
            Dim oldValue As String = String.Empty
            Dim newValue As String = String.Empty
            Dim dtOldValue As New DataTable
            Dim dtNewValue As New DataTable
            Dim sqlDataAdapter As New SqlDataAdapter(cmd)

            If sqlMode = sqlMode.INSERT Then
                'insert patient
                'SECTION 3
                cmd.CommandText = String.Format("INSERT INTO myPatient({0}) VALUES ({1})", sbAffectedDBFields.ToString, sbAffectedDBFields.Replace(",", ",@").ToString)
                cmd.ExecuteNonQuery()

            ElseIf sqlMode = sqlMode.UPDATE Then
                'get old value before update
                cmd.CommandText = String.Format("SELECT {0} FROM myPatient WHERE PCNO=@PCNO", sbAffectedDBFields.ToString)
                sqlDataAdapter.SelectCommand.Parameters.AddWithValue("@PCNO", patient.PCNO)
                sqlDataAdapter.Fill(dtOldValue)

                'dtOldValue will have only one row always
                'SECTION 4
                For Each dr As DataRow In dtOldValue.Rows
                    For Each dc As DataColumn In dtOldValue.Columns
                        oldValue &= dr(dc).ToString & ","
                    Next
                Next
                oldValue.Remove(oldValue.Length - 1, 1) 'to remove last ","

                'update patient
                'updateString variable in a format of (field1=@field1,field2=@field2,fieldN=@fieldN)
                cmd.CommandText = String.Format("UPDATE myPatient SET {0} WHERE PCNO=@PCNO", updateString)
                cmd.ExecuteNonQuery()
            End If

            'get new value after update
            cmd.CommandText = String.Format("SELECT {0} FROM myPatient WHERE PCNO=@PCNO", sbAffectedDBFields.ToString)
            sqlDataAdapter.SelectCommand.Parameters.AddWithValue("@PCNO", patient.PCNO)
            sqlDataAdapter.Fill(dtNewValue)

            'dtNewValue will have only one row always
            For Each dr As DataRow In dtNewValue.Rows
                For Each dc As DataColumn In dtNewValue.Columns
                    newValue &= dr(dc).ToString & ","
                Next
            Next
            newValue.Remove(newValue.Length - 1, 1) 'to remove last ","

            'param tableName,affectedColumn,oldvalue,newvalue
            Audit("myPatient", sbAffectedDBFields.ToString, oldValue, newValue)

            Return True
        Catch ex As Exception
            ErrorLog.Log(ex, ErrorLog.LogLevel.Message, MethodBase.GetCurrentMethod())
            Return False
        End Try
    End Using
End Function

Answer 1

I think it's way too long when I have to list out most of the fields. Is there any better way of doing this?

Extract the creation of the parameters in a method passing in the Patient object and the SqlParameterCollection of the SqlCommand object

Private Sub FillPatientParameterCollection(patient As Patient, parameters As SqlClient.SqlParameterCollection)

    Dim currentDate As Date = Now

    With parameters
        .AddWithValue("@PCNO", patient.PCNO)
        .AddWithValue("@PName", patient.PName)
        .AddWithValue("@NRICType", patient.NRICType)
        .AddWithValue("@NRIC", patient.NRIC)
        'Another 80+ fields
        .AddWithValue("@Created_Date", currentDate)
        .AddWithValue("@Created_By", patient.CreateOrEditBy)
        .AddWithValue("@Edit_Date", currentDate)
        .AddWithValue("@Edit_By", patient.CreateOrEditBy)
        .AddWithValue("@FirstSeen", currentDate)
    End With

End Sub

Also see the currentDatevariable. This ensures that the values for Created_Date,Edit_Date... are the same.

call the method like

FillPatientParameterCollection (patient, cmd.Parameters)

I build up the DB column name so it can be used later in my select and insert statement.

This should also be extracted in a own method but made accessible by a property. In this way it needs to be filled only once. This method should return a List(Of String) as this is easier to read and to maintain.

Private _AffectedDBFields As List(Of String)
Private ReadOnly Property AffectedDBFields As List(Of String)
    Get
        If (_AffectedDBFields.Count = 0) Then
            _AffectedDBFields = GetAffectedDBFields()
        End If
        Return _AffectedDBFields
    End Get
End Property
Private Function GetAffectedDBFields() As List(Of String)
    Dim affectedFields As New List(Of String)()
    affectedFields.Add("PCNO")
    affectedFields.Add("PName")
    affectedFields.Add("NRICType")
    affectedFields.Add("NRIC")
    affectedFields.Add("Anoter 80+ fields")
    affectedFields.Add("Created_Date")
    Return affectedFields
End Function  

This is where and how I use the string I build up in Section 2. Does it look good or ugly?

As you don't have a StringBuilderobject anymore you need to use the String.Join() method.

So this

cmd.CommandText = String.Format("INSERT INTO myPatient({0}) VALUES ({1})", sbAffectedDBFields.ToString, sbAffectedDBFields.Replace(",", ",@").ToString)

will become this

cmd.CommandText = String.Format("INSERT INTO myPatient({0}) VALUES ({1})", String.Join(",", AffectedDBFields), String.Join(",@", AffectedDBFields))

Audit is required after inserting/update a new record, therefore I need to get the old and new value. There should be a better way to do this I guess, but I just have no idea how.

You should extract the loop over the dtOldValues.Rows and also dtNewValues.Rows to a method, which again can return a List(Of String) instead of a String

Private Function GetDataRowValues(row As DataRow) As List(Of String)
    Dim values As New List(Of String)()
    For Each dc As DataColumn In row.Table.Columns
        Dim value As String = String.Empty
        If (Not IsDBNull(row(dc))) Then
            value = row(dc).ToString()
        End If
        values.Add(value)
    Next
    Return values
End Function

You then call this method with a guard condition like this

If dtOldValue.Rows.Count > 0 then
    oldValue = String.Join(",",GetDataRowValues(dtOldValues.Rows(0)))
End If

and

If dtNewValue.Rows.Count > 0 then
    newValue = String.Join(",",GetDataRowValues(dtNewValues.Rows(0)))
End If

At the end you need to call your Audit() like

Audit("myPatient", String.Join(",", AffectedDBFields), oldValue, newValue)

Additional remarks

I have integrated a check for DbNull values in the GetDataRowValues() method. You should always check for DbNull values.