# simple transaction process

I ask if the following code need some refactoring :

public static int InsertBonus(DataTable dt, int month, int year)
{

int affectedRow = -1;
using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["xxx"].ToString()))
{
using (IfxTransaction tran = con.BeginTransaction())
{
try
{
StringBuilder cmdTxt = new StringBuilder();

cmdTxt.Append(" DELETE FROM rh7uiw WHERE bonus_mon = ? AND bonus_year = ? ");
using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
{
myIfxCmd.CommandType = CommandType.Text;
if (con.State == ConnectionState.Closed)
{
con.Open();
}
myIfxCmd.Parameters[0].Value = ((object)month) ?? DBNull.Value;
myIfxCmd.Parameters[1].Value = ((object)year) ?? DBNull.Value;
affectedRow = myIfxCmd.ExecuteNonQuery();
}

cmdTxt.Length = 0;
cmdTxt.Append(" INSERT INTO rh7uiw (emp_num,bonus_year,bonus_mon,bonus_value,bonus_no,rr_code) VALUES(?,?,?,?,?,?) ");

using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
{
myIfxCmd.CommandType = CommandType.Text;
foreach (DataRow r in dt.Rows)
{
if (con.State == ConnectionState.Closed)
{
con.Open();
}
myIfxCmd.Parameters[0].Value = (r["emp_num"]) ?? DBNull.Value;
myIfxCmd.Parameters[1].Value = (r["year"]) ?? DBNull.Value;
myIfxCmd.Parameters[2].Value = (r["month"]) ?? DBNull.Value;
myIfxCmd.Parameters[3].Value = (r["bonus_al"]) ?? DBNull.Value;
myIfxCmd.Parameters[4].Value = 1;
myIfxCmd.Parameters[5].Value = 1;
affectedRow = myIfxCmd.ExecuteNonQuery();
}
}
tran.Commit();
con.Close();
con.Dispose();
return affectedRow;

}
catch
{
tran.Rollback();
throw;
}
}
}
}


Yes:

• You don't need to use a StringBuilder when you are not building a string. Just use the string.
• Don't check the state of the connection for every query. Just open the connection before the first query, and it stays open.
• The check for null for month and year in the first query is not needed. An int can not be null.
• As you are creating the connection in a using block, you don't need to close and dispose it. The using block will do that for you.
• Returning affectedRow from the method seems pointless, as it only contains how many records the last query affected.

I totally agree with Guffa, but I want to add some things: I don't like so long methods, because they are really hard to read, especially for other people which might maintain you code in the future. So I would create some private methods with meaningful names. So everything within the try block, I would change to something like this (pseudocode):

try{
ExcecuteDelete(parameters, ...)
ExcecuteInsert(parameters,...)
}


So you have three short instead of one long method. The query text ("DELETE FROM rh7uiw WHERE bon...") seems to be constant, so I would like to make it a private static constant class variable.

Furthermore, don't use any prefix like my, our and so on, as far as they don't have a special meaning in the context they are used for variable names. Simply call them ifxCommand, command or better: deleteCommand and insertCommand.

By the way, why don't you use a updateCommand? If the affected rows are 0 after a update, you know you have to insert new stuff. But in most cases you have only one database query, instead of always two.