# Long list of SQL parameters and audits

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
'Another 80+ fields
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

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)

'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)

'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


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
'Another 80+ fields
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)()
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
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)