simple transaction process

Question

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;
                    myIfxCmd.Parameters.Add("bonus_mon", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_year", IfxType.Integer);
                    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;
                    myIfxCmd.Parameters.Add("emp_num", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_year", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_mon", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_value", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_no", IfxType.Integer);
                    myIfxCmd.Parameters.Add("rr_code", IfxType.Integer);
                    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;
            }
        }
    }
}

Answer 1

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.

Answer 2

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.