Transaction handling for multiple SQL statements

Question

In this code I update two tables called Payment and SalaryTrans. First I insert records (Salary payments) to Payment table and then I update SalaryTrans table. When a record is inserted to Payment table, ID is auto generated (auto increment column) and then that ID should be updated in SalaryTrans table (for all matching records).

The following code compiles and runs giving the desired output, but I just want to know whether the transactions are handled properly. Also, are there any other flows in this code?

Could you please review and give your feedback?

public int InsertPaymentDetails(List<PaymentInfo> list, int totalRecords, decimal totalAmount )
{ 
 const string selectSatement = @"INSERT INTO Payment (payment_type, reference, payment_date, total_records, total_amount) 
VALUES(@type, @reference, @date, @totalRecords, @totalAmount ) ";
 const string updateStatement = @"UPDATE SalaryTrans SET payment_id = (SELECT TOP 1 id FROM Payment ORDER BY Payment.id DESC) 
WHERE SalaryTrans.employee_id = @employeeID AND SalaryTrans.reference = @reference ";
 //An item is refered in the list just to get the PaymentType, PaymentDate etc. as these information are common for all items in the list
 var first = 0;
 var payInfo = list[first];
 //To get the affected rows following variables are declared
 int result= 0;
 int affectedRecords = 0;
 using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
 {
 sqlConnection.Open();
 using (SqlTransaction sqlTrans = sqlConnection.BeginTransaction ())
 using (SqlCommand sqlCommand = new SqlCommand(selectSatement, sqlConnection,sqlTrans))
 {
 sqlCommand.Parameters.Add("@type", SqlDbType.VarChar).Value = payInfo.PaymentType;
 sqlCommand.Parameters.Add("@date", SqlDbType.Date).Value = payInfo.PaymentDate;
 sqlCommand.Parameters.Add("@reference", SqlDbType.VarChar).Value = payInfo.Reference;
 sqlCommand.Parameters.Add("@totalRecords", SqlDbType.Int).Value = totalRecords;
 sqlCommand.Parameters.Add("@totalAmount", SqlDbType.Decimal).Value = totalAmount;
 sqlCommand.ExecuteNonQuery();
 SqlParameter paramEmployeeID = new SqlParameter("@employeeID", SqlDbType.Int);
 sqlCommand.Parameters.Add(paramEmployeeID);
 foreach (PaymentInfo p in list)
 {
 paramEmployeeID.Value = p.EmployeeID;
 sqlCommand.CommandText = updateStatement; //Command text changed to update second table
 result = sqlCommand.ExecuteNonQuery();
 //If at least one recored was inserted then the recordsAffected should be +1
 if (result == +1)
 affectedRecords = 1;
 }
 sqlTrans.Commit();
 return affectedRecords;
 }
 }
}

• Paritosh: so you are adding one payment and updating salarytrans for all payment. does not makes sense.
• CAD: @paritosh, One payment means its a summary.(Total no of employees, Payment date, Total amount etc.) After entering the summary record, its ID is updated in SalaryTrans table. So that when we refer a record in SalaryTrans we know what is the related summary record in Payment table.

Answer 1

Naming

Constants in .NET should be named using PascalCase. See c-sharp-naming-convention-for-constants
Your const string selectSatement = @"INSERT INTO Payment..." should be better named InsertStatement to also reflect that it is an INSERT and not a SELECT.

var first = 0;
var payInfo = list[first]; 

As the value of var first won't be changed, you should change it to a const with a meaningful name.

Refactoring
Before using the first item in the list, a check to see if the list is null or empty should take place.

const int NoRowsAffected = 0;
if (list == null || !list.Any()) { return NoRowsAffected; } 

int result= 0; 

won't be used, except for assigning the affetced rows of the update command. we should just remove it.

if (result == +1)
 affectedRecords = 1; 

Using curly brackets {} should be a have to everytime, at least if written on separate lines.

sqlCommand.CommandText = updateStatement;

should be placed outside of the for loop. Ther is no need to assign the const to the CommandText property for each iteration.

Result

public int InsertPaymentDetails(List<PaymentInfo> paymentInfos, int totalRecords, decimal totalAmount)
{
 const string InsertStatement = @"
 INSERT INTO Payment (payment_type, reference, payment_date, total_records, total_amount) 
 VALUES(@type, @reference, @date, @totalRecords, @totalAmount )
 ";
 const string UpdateStatement = @"
 UPDATE SalaryTrans 
 SET payment_id = (SELECT TOP 1 id FROM Payment ORDER BY Payment.id DESC) 
 WHERE SalaryTrans.employee_id = @employeeID 
 AND SalaryTrans.reference = @reference 
 ";
 const int NoRowsAffected = 0;
 const int FirstListIndex = 0;
 if (paymentInfos == null || !paymentInfos.Any()) { return NoRowsAffected; }
 var payInfo = paymentInfos[FirstListIndex];
 //To get the affected rows following variables are declared
 int affectedRecords = NoRowsAffected;
 using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
 {
 sqlConnection.Open();
 using (SqlTransaction sqlTrans = sqlConnection.BeginTransaction())
 using (SqlCommand sqlCommand = new SqlCommand(InsertStatement, sqlConnection, sqlTrans))
 {
 sqlCommand.Parameters.Add("@type", SqlDbType.VarChar).Value = payInfo.PaymentType;
 sqlCommand.Parameters.Add("@date", SqlDbType.Date).Value = payInfo.PaymentDate;
 sqlCommand.Parameters.Add("@reference", SqlDbType.VarChar).Value = payInfo.Reference;
 sqlCommand.Parameters.Add("@totalRecords", SqlDbType.Int).Value = totalRecords;
 sqlCommand.Parameters.Add("@totalAmount", SqlDbType.Decimal).Value = totalAmount;
 sqlCommand.ExecuteNonQuery();
 SqlParameter paramEmployeeID = new SqlParameter("@employeeID", SqlDbType.Int);
 sqlCommand.Parameters.Add(paramEmployeeID);
 sqlCommand.CommandText = UpdateStatement;
 foreach (PaymentInfo paymentInfo in paymentInfos)
 {
 paramEmployeeID.Value = paymentInfo.EmployeeID;
 //If at least one recored was inserted then the recordsAffected should be +1
 if (sqlCommand.ExecuteNonQuery() == 1) { affectedRecords = 1; }
 }
 sqlTrans.Commit();
 }
 }
 return affectedRecords;
}

• TyCobb: Not to go down the rabbit hole of naming conventions, but I believe local constants get the lowerCamelCase treatment. Certain style cops will attempt to force this (ReSharper being one). Unfortunately this is the only link source I could find to something written down. stackoverflow.com/questions/3157431/…
• Dan Lyons: Since you already suggested he use some LINQ functionality to make sure the list is non-empty, he could replace the var payInfo = list[first]; with var payInfo = list.First(); and drop first entirely. He could then change the list parameter to IEnumerable<PaymentInfo>.

Answer 2

SQL readability

This is minor but good to keep in mind. C# reads like crap if it's all in-line, and so does SQL. Try this:

{ 
 const string selectSatement = @"
 INSERT INTO Payment (payment_type, reference, payment_date, total_records, total_amount) 
 VALUES(@type, @reference, @date, @totalRecords, @totalAmount ) 
 ";
 const string updateStatement = @"
 UPDATE SalaryTrans 
 SET payment_id = (SELECT TOP 1 id FROM Payment ORDER BY Payment.id DESC) 
 WHERE SalaryTrans.employee_id = @employeeID 
 AND SalaryTrans.reference = @reference 
 ";

Answer 3

It's odd that selectSatement is actually an INSERT. The misnamed and misspelled variable leaves me unsated.

The UPDATEs contain a side query to find the greatest id in the Payment table. Presumably, you want to find the id that was auto-generated by the INSERT. The correct way to do that in SQL Server involves an OUTPUT clause on the INSERT operation. As you've currently written it, you are probably vulnerable to a race condition, since other concurrent transactions may have inserted more rows in the Payment table between your INSERT and UPDATEs. The exact behaviour probably depends on the transaction isolation level being used.

• hangy: ADO.NET has implicit connection pooling for MSSQL so it is a good idea to Open/Close a SqlConnection close to where it is used.

Answer 4

Your data model has a small problem regarding data consistency. If someone were to update a SalaryTrans.reference to a different value, it would be different than the one in a Payment record that's joined through the payment_id foreign key. Likewise, total_records and total_amount should probably be equal to the values that could be retrieved by aggregating SalaryTrans. A possible solution for the reference would be to introduce a SalaryTransReference table and have both SalaryTrans and Payment reference that with a foreign key. (Make it a unique key on the Payment table if there can only be one payment for a single reference.) I would also consider a PaymentType table instead of having a payment_type varchar(...) column on the Payment table for similar reasons.

The call to ExecuteNonQuery() inside the foreach loop contains too many parameters. I do not know if the SqlCommand knows how to purge unused parameters, but I would definitely clear the parameter list before adding setting the command to this UPDATE statement, and then add the required parameters.

Also, the method is public and makes a lot of assumptions about the data passed in the list without checking them. A different caller of that method might pass in mixed payment_types and expect it to work. This could lead to unexpected results. I do not think changing this up to support mixed PaymentInfo objects should yield in a huge performance degradation. It is also a bit weird that the result of the method is int, but always 0 or 1, irrelevant of the number of actually inserted/updated rows. In the current model, I would prefer a bool result.

Answer 5

The most egregious problem that I see here is that this method is a jungle of different actions and concerns. It should look like this:

using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
{
 sqlConnection.Open();
 using (SqlTransaction sqlTrans = sqlConnection.BeginTransaction())
 {
 ExecuteInsert(sqlConnection, sqlTrans, ...);
 var hasAnyUpdate = false;
 foreach (PaymentInfo p in list)
 {
 var hasUpdated = ExecuteUpdate(sqlConnection, sqlTrans, p);
 if (hasUpdated)
 hasAnyUpdate = true;
 }
 sqlTrans.Commit();
 return hasAnyUpdate;
 }
}

And now you can see what this method does.

I found it to be particularly unfortunate that the sqlCommand in the original code was overloaded to do many things. In fact the later statements are all sending parameters which are not used.

All the connection management and transaction handling should be extracted as well, such that you can write:

using (var dbSession = CreateDatabaseSession()) {
 ExecuteInsert(dbSession.Connection, dbSession.Transaction, ...);
 dbSession.Commit();
}

Remove infrastructure concerns from the logic part of your code.