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.