3
\$\begingroup\$

I just want to know whether I've done transaction handling well. Also what about the other types of transactions available in .NET with regard to this code snippet?

Also are there any redundant things in this code?

    public void InsertAttendance(BindingList<IAttendance> attendanceList)
    {
        string selectStatement = @"IF NOT EXISTS (SELECT Attendance.Atten_ID FROM Attendance WHERE Atten_ID = @aid) BEGIN INSERT INTO attendance (EID,PID,in_time,out_time,shift) VALUES(@eid,@pid,@inT,@outT,@shift) END ";
        using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
        {
            sqlConnection.Open();
            using (SqlTransaction trans = sqlConnection.BeginTransaction())
            using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection, trans))
            {
                sqlCommand.Parameters.Add("@aid", SqlDbType.Int);
                sqlCommand.Parameters.Add("@pid", SqlDbType.Char);
                sqlCommand.Parameters.Add("@eid", SqlDbType.Int);
                sqlCommand.Parameters.Add("@inT", SqlDbType.DateTime);
                sqlCommand.Parameters.Add("@outT", SqlDbType.DateTime);
                sqlCommand.Parameters.Add("@shift", SqlDbType.Char);
                //sqlConnection.Open();

                try
                {
                    foreach (Attendance obj in attendanceList)
                    {
                        sqlCommand.Parameters["@aid"].Value = obj.AttendanceID;
                        sqlCommand.Parameters["@pid"].Value = obj.PointID;
                        sqlCommand.Parameters["@eid"].Value = obj.EmployeeID;
                        sqlCommand.Parameters["@inT"].Value = obj.InTime;
                        sqlCommand.Parameters["@outT"].Value = obj.OutTime;
                        sqlCommand.Parameters["@shift"].Value = obj.ShiftType;

                        sqlCommand.ExecuteNonQuery();
                    }
                    trans.Commit();
                }
                catch { trans.Rollback(); throw; }
            }
        }
    }
\$\endgroup\$
2
\$\begingroup\$

First and foremost, I would strongly suggest using an ORM (e.g., Entity Framework or nHibernate) rather than doing all this manual work to update the database. ORMs take most of the drudge work away and generally make things much easier.

That aside, there are a few things I would change:

  • As much as possible, utilize the ADO.NET interface methods instead of creating concrete classes directly
  • While a commit must be performed before leaving scope, rollback happens automatically, so the catch block is unnecessary
  • The method should not take in BindingList, both because it only requires IEnumerable and because DAL code shouldn't have ties to view concerns
  • The SQL statement variable should be const
  • There is no need to prefix variables with sql - this smells like a form of Hungarian Notation

With that in mind, I would modify it to be more like the following:

public void InsertAttendance(IEnumerable<IAttendance> attendanceList)
{
    const string selectStatement = @"IF NOT EXISTS (SELECT Attendance.Atten_ID FROM Attendance WHERE Atten_ID = @aid) BEGIN INSERT INTO attendance (EID,PID,in_time,out_time,shift) VALUES(@eid,@pid,@inT,@outT,@shift) END ";
    using (var connection = new SqlConnection(db.ConnectionString))
    {
        connection.Open();
        using (var trans = connection.BeginTransaction())
        using (var command = connection.CreateCommand())
        {
            command.CommandText = selectStatement;
            var parameters = command.Parameters;

            parameters.Add("@aid", SqlDbType.Int);
            parameters.Add("@pid", SqlDbType.Char);
            parameters.Add("@eid", SqlDbType.Int);
            parameters.Add("@inT", SqlDbType.DateTime);
            parameters.Add("@outT", SqlDbType.DateTime);
            parameters.Add("@shift", SqlDbType.Char);

            foreach (var attendanceItem in attendanceList)
            {
                parameters["@aid"].Value = attendanceItem.AttendanceID;
                parameters["@pid"].Value = attendanceItem.PointID;
                parameters["@eid"].Value = attendanceItem.EmployeeID;
                parameters["@inT"].Value = attendanceItem.InTime;
                parameters["@outT"].Value = attendanceItem.OutTime;
                parameters["@shift"].Value = attendanceItem.ShiftType;

                command.ExecuteNonQuery();
            }
            trans.Commit();
        }
    }
}

Sadly, creating parameters is particularly painful using just IDbCommand, so I continued using the SqlParameterCollection members.

Additional questions/considerations:

  • Should PointID be char? An ID suggests a numeric type.
  • Should ShiftType be char? This name suggests an enum type, which I suppose you could choose to represent as a character in the database, but it could also be a numeric type.
  • The SQL statement no-ops if a record already exists, but the method gives no indication when this occurs. This should probably be re-considered, since it fails to provide any feedback to the caller that the insert did not occur.
  • What is db? It seems to be providing the connection string, but we only see the property reference.
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the quick and comprehensive review. With regard to questions... Actually Point id has a character prefix for example like "A001". Shift type would be "D" (Day) or "N" (Night). To get a feedback I'm thinking to use command.affectedRows property. Yes db is a class and it's task is to build the connection string from app.config. \$\endgroup\$ – CAD Jul 24 '14 at 4:15

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.