1
\$\begingroup\$

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;
            }
        }
    }
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

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.
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.