-2
\$\begingroup\$

The codes below is in DLL.

Here's the problems that I can identify:

1) Caller won't know what's the exception?

2) Catch exception 1 but throw exception 2.

3) num is not sum but overwrite.

What else?

These codes are in production, I just found it not well written.

I would like to get more feedback and is there any other way to improve it?

public int ExecuteTransactionBatchSQL(ref string[] arrSqlStatement)
{
  int num = -1;
  this.myCn = new SqlConnection(this._ConnectionString);
  SqlTransaction sqlTransaction = (SqlTransaction) null;
  try
  {
    this.myCn.Open();
    sqlTransaction = this.myCn.BeginTransaction("MyTransaction");
    this.myCmd = new SqlCommand();
    this.myCmd.Connection = this.myCn;
    this.myCmd.Transaction = sqlTransaction;
    for (int index = 0; index < arrSqlStatement.Length && (arrSqlStatement[index] != null && string.Compare(arrSqlStatement[index], string.Empty) != 0); ++index)
    {
      this.myCmd.CommandText = arrSqlStatement[index];
      num = this.myCmd.ExecuteNonQuery();
    }
    sqlTransaction.Commit();
  }
  catch (Exception ex1)
  {
    num = -1;
    try
    {
      if (sqlTransaction != null)
        ((DbTransaction) sqlTransaction).Rollback();
    }
    catch (Exception ex2)
    {
      throw ex2;
    }
  }
  finally
  {
    if (this.myCmd != null)
      this.myCmd.Dispose();
    if (this.myCn != null)
      this.myCn.Close();
  }
  return num;
}
\$\endgroup\$
  • \$\begingroup\$ Did you write or do you have to maintain this code ? \$\endgroup\$ – Heslacher Oct 5 '15 at 6:07
  • \$\begingroup\$ maintain, it drive me crazy. \$\endgroup\$ – Pop Oct 5 '15 at 6:08
  • \$\begingroup\$ If you're sure it's working, then you don't need an uncertain title as such. Only have it state the code's purpose. \$\endgroup\$ – Jamal Oct 5 '15 at 6:12
  • \$\begingroup\$ Your language is a little unclear, but are you saying you've identified those problems. And now you'd like to get more feedback on those and other ways you could improve this code? \$\endgroup\$ – SuperBiasedMan Oct 5 '15 at 9:05
  • \$\begingroup\$ Yes. I thought those are implied on code review. \$\endgroup\$ – Pop Oct 5 '15 at 9:41
1
\$\begingroup\$
  • SqlConnection, SqlTransaction and SqlCommand are implementing the IDispoable interface and should therefor be enclosed in a using statement. This makes sure that the objects are properly disposed.

  • why is the string[] arrSqlStatement passed with the ref keyword ?

  • this condition (arrSqlStatement[index] != null && string.Compare(arrSqlStatement[index], string.Empty) != 0) in the loops header makes the code almost unreadable. A much better way would be to use the built in string method string.IsNullOrWhiteSpace(), but much better would be to first filter these invalid statements by using a linq query.

  • I assume that on the callers side there will be some kind of exception handling so I would skip this whole try..catch's. You can't handle the exception, so there is no need for it. Having a return of num = -1 doesn't have enough value that these try..catch's should be there.

  • The reuse of class owned objects like this.myCn and this.myCmd will make your code in this case non thread safe if the object is passed to several other objects. Creating new objects in the method itself is much better.

Applying these points will lead to

public int ExecuteTransactionBatchSQL(ref string[] arrSqlStatement)
{
    IEnumerable<string> sqlStatements = arrSqlStatement.Where(s => !string.IsNullOrWhiteSpace(s));

    if (!sqlStatements.Any()) { return -1; }

    int num = -1;

    using (SqlConnection connection = new SqlConnection(this._ConnectionString))
    {
        connection.Open();
        using (SqlTransaction sqlTransaction = connection.BeginTransaction("MyTransaction"))
        using (SqlCommand sqlCommand = new SqlCommand() { Connection = connection, Transaction = sqlTransaction })
        {
            foreach(string statement in sqlStatements)
            {
                sqlCommand.CommandText = statement;
                num = sqlCommand.ExecuteNonQuery();
            }

            sqlTransaction.Commit();

        }
    }

    return num;
}
| improve this answer | |
\$\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.