current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I have written the following method twice but I don't know which is better from performance perspective, code design and best practice.

First:

public int Insert()
        {
            int affectedRow = -1;
            using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
            {

                StringBuilder cmdTxt = new StringBuilder();
                cmdTxt.Append(" INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ");
                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("day", IfxType.Char);
                    myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);

                    if (con.State == ConnectionState.Closed)
                    {
                        con.Open();
                    }

                    myIfxCmd.Parameters[0].Value = ((object)this.DayId) ?? DBNull.Value;
                    myIfxCmd.Parameters[1].Value = ((object)this.ShortName) ?? DBNull.Value;
                    myIfxCmd.Parameters[2].Value = ((object)this.Name) ?? DBNull.Value;
                    myIfxCmd.Parameters[3].Value = this.DepCode;
                    myIfxCmd.Parameters[4].Value = this.StudyCode;
                    myIfxCmd.Parameters[5].Value = this.BatchNum;

                    affectedRow = myIfxCmd.ExecuteNonQuery();
                }
            }
            return affectedRow;
        }

THEN :

   foreach (Day a in days)
    {
        affectedRow = a.Insert();
    }

Second:

public int Insert(List<Day> days)
        {
            int affectedRow = -1;
            using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
            {

                StringBuilder cmdTxt = new StringBuilder();
                cmdTxt.Append(" INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ");
                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("day", IfxType.Char);
                    myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);

                    if (con.State == ConnectionState.Closed)
                    {
                        con.Open();
                    }
                    foreach (Day a in days)
                    {
                        myIfxCmd.Parameters[0].Value = ((object)a.DayId) ?? DBNull.Value;
                        myIfxCmd.Parameters[1].Value = ((object)a.ShortName) ?? DBNull.Value;
                        myIfxCmd.Parameters[2].Value = ((object)a.Name) ?? DBNull.Value;
                        myIfxCmd.Parameters[3].Value = a.DepCode;
                        myIfxCmd.Parameters[4].Value = a.StudyCode;
                        myIfxCmd.Parameters[5].Value = a.BatchNum;

                        affectedRow = myIfxCmd.ExecuteNonQuery();
                    }
                }

            }

            return affectedRow;
        }
share|improve this question
up vote 6 down vote accepted

In general, the best is to do all your inserts in one connection.

Open/Close a connection implies a penalty. Although it is mitigated by some connection pooling mechanism on the database side (btw what is it ?), it is additional work that is not useful nor required.

The reason is that there is the possibility to use a Transaction to ensure that "all or nothing" is inserted, regarding whether things goes good or bad in your insertion loop, and in most cases a transaction is tied to a connection.

Here is something that includes a transaction. That'the way I am doing it in SQLServer. I assumed that the Informix transaction are correctly implemented using the ADO.NET scheme, and I do not know is this implementation is working.

public int Insert(List<Day> days)
{
    int affectedRow = -1;
    using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
    {
        con.Open();
        using(var transaction = con.BeginTransaction())
        {
            var cmdTxt = " INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ";
            using (var myIfxCmd = new IfxCommand(cmdTxt, con,transaction ))
            {
                myIfxCmd.CommandType = CommandType.Text;
                myIfxCmd.Parameters.Add("day", IfxType.Char);
                myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);



                foreach (Day a in days)
                {
                    myIfxCmd.Parameters[0].Value = ((object)a.DayId) ?? DBNull.Value;
                    myIfxCmd.Parameters[1].Value = ((object)a.ShortName) ?? DBNull.Value;
                    myIfxCmd.Parameters[2].Value = ((object)a.Name) ?? DBNull.Value;
                    myIfxCmd.Parameters[3].Value = a.DepCode;
                    myIfxCmd.Parameters[4].Value = a.StudyCode;
                    myIfxCmd.Parameters[5].Value = a.BatchNum;

                    affectedRow = myIfxCmd.ExecuteNonQuery();
                }
            }

            // commit the changes in the database. 
            // if an exception occurs somewhere, then the using blocks
            // makes that the transaction is disposed and rolled back,
            // and nothing will be updated in the database
            transaction.Commit();
        }
    }

    return affectedRow;
}
share|improve this answer
    
Thanks a lot ,Could you refactor my code with transaction and the notes you think it's useful please? – Anyname Donotcare Mar 21 at 10:01
1  
Ok, gimme just an hour left and I will do that. – Larry Mar 21 at 10:23
    
i will be grateful ,my DB is informix :) – Anyname Donotcare Mar 21 at 10:25
    
Well I removed the StringBuilder. It is not worth using a StringBuilder that way, because there is no massive string concatenations. – Larry Mar 21 at 10:44
1  
I don't see why not :) – Larry Mar 21 at 12:47
up vote 4 down vote

Why do you employ a StringBuilder when you don't do anything with it? Especially when you should be using it. Right now the names of the various fields are defined in two places: 

mdTxt.Append(" INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ");

And also:

myIfxCmd.Parameters.Add("day", IfxType.Char);
myIfxCmd.Parameters.Add("short", IfxType.NVarChar);

If one of them changes, you now have to change its name in two places, which is a recipe for disaster.

Also note that shedule contains a typo, it should be schedule.)

This is another accident waiting to happen:

myIfxCmd.Parameters[0].Value = ((object)this.DayId) ?? DBNull.Value;
myIfxCmd.Parameters[1].Value = ((object)this.ShortName) ?? DBNull.Value;
myIfxCmd.Parameters[2].Value = ((object)this.Name) ?? DBNull.Value;
myIfxCmd.Parameters[3].Value = this.DepCode;
myIfxCmd.Parameters[4].Value = this.StudyCode;
myIfxCmd.Parameters[5].Value = this.BatchNum;

What if the order changes? Also, this makes it hard to figure out which parameter is connected to which field.

share|improve this answer
    
Hi. Unfortunately, this is the way OleDb is working in ADO.NET. Parameters are ordered and do not care about names. I know, it is a pain in the a**. – Larry Mar 21 at 10:21
1  
Thanks a lot , but you don't answer the original question .Could you refactor the method in the way you did think it will be more better to use . – Anyname Donotcare Mar 21 at 10:24
    
@Larry True, but that still doesn't change the fact that it can be done a lot better. – BCdotWEB Mar 21 at 10:28
1  
Your comment about names is simply not true, as the field names and the parameter names are not tied together and could be different. A code review should be about the aspects of the code that you understand – edc65 Mar 21 at 16:58
1  
@edc65 From what I gather the parameter name does seem to be completely useless yet required. In the code the parameter and the field name correspond (not a bad idea), so it would be good to ensure they actually stay coupled. – BCdotWEB Mar 22 at 11:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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