Search code examples
c#sqlcompact-frameworksql-server-cewindows-ce

Should I refactor this, or is my confusion cause to be cautious?


This SqlCe code looks awfully strange to me:

cmd.CommandText = "INSERT INTO departments ( account_id, name) VALUES (?, ?)";
foreach(DataTable tab in dset.Tables)
{
    if (tab.TableName == "Departments")
    {
        foreach(DataRow row in tab.Rows)
        {
            Department Dept = new Department();
            if (!ret)
                ret = true;
            foreach(DataColumn column in tab.Columns)
            {

                if (column.ColumnName == "AccountID")
                {
                    Dept.AccountID = (string) row[column];
                }
                else if (column.ColumnName == "Name")
                {
                    if (!row.IsNull(column))
                        Dept.AccountName = (string) row[column];
                    else
                        Dept.AccountName = "";
                }
            }
            List.List.Add(Dept);
            . . .
            dSQL = "INSERT INTO departments ( account_id, name) VALUES ('" + Dept.AccountID + "','" + Dept.AccountName +"')";

            if (!First)
            {
                cmd.Parameters[0].Value = Dept.AccountID;
                cmd.Parameters[1].Value = Dept.AccountName;
            }

            if (First)
            {
                cmd.Parameters.Add("@account_id",Dept.AccountID);
                cmd.Parameters.Add("name",Dept.AccountName);
                cmd.Prepare();
                First = false;
            }

            if (frmCentral.CancelFetchInvDataInProgress)
            {
                ret = false;
                return ret;
            }

            try
            {
                dbconn.DBCommand( cmd, dSQL, true );
            } 
            . . .

    public void DBCommand(SqlCeCommand cmd, string dynSQL, bool Silent)
    {
        SqlCeTransaction trans = GetConnection().BeginTransaction();
        cmd.Transaction = trans;
        try
        {
            cmd.ExecuteNonQuery();
            trans.Commit();
        }
        catch (Exception ex)
        {
            try 
            {
                trans.Rollback();
            }
            catch (SqlCeException) 
            {
                // Handle possible exception here
            }
            MessageBox.Show("DBCommand Except 2"); // This one I haven't seen...
            WriteDBCommandException(dynSQL, ex, Silent);
        }
    }

My questions are:

1) Should "?" really be used in the assignment to cmd.CommandText, or should "@" be used instead?

2) One of the "cmd.Parameters.Add()"s (account_id) uses a "@" and the other (name) doesn't. Which way is right, or is the "@" optional?

3) I can't make heads or tails of why DBCommand() is written as it is - the final two args are only used if there's an exception...???

I'm tempted to radically refactor this code, because it seems so bizarre, but since I don't really understand it, that might be a recipe for disaster...


Solution

  • The ? parameter is older Access syntax.

    My guess is this used to be an Access database, but someone converted it to SQL CE at some point.

    Generally, SQL understands that ? parameter, but it's better to just change that while you are in there so that it is more understood.

    I'm still trying to make heads & tails of all these variables. If I get it sorted out, I'll post up compilable (sp?) code.

    EDIT: I had to put this into a method and work out all of the RED errors to make sure I wasn't giving you something that would not compile.

    I passed it your DataSet like so, with lots of comments added:

    private bool StrangeSqlCeCode(DataSet dset) {
      const string ACCOUNT_ID = "AccountID";
      const string DEPARTMENTS = "Departments";
      const string NAME = "Name";
      const string SQL_TEXT = "INSERT INTO departments (account_id, name) VALUES (@account_id, @name)";
      bool ret = false;
      //bool First = false; (we don't need this anymore, because we initialize the SqlCeCommand correctly up front)
      using (SqlCeCommand cmd = new SqlCeCommand(SQL_TEXT)) {
        // Be sure to set this to the data type of the database and size field
        cmd.Parameters.Add("@account_id", SqlDbType.NVarChar, 100);
        cmd.Parameters.Add("@name", SqlDbType.NVarChar, 100);
        if (-1 < dset.Tables.IndexOf(DEPARTMENTS)) {
          DataTable tab = dset.Tables[DEPARTMENTS];
          foreach (DataRow row in tab.Rows) {
            // Check this much earlier. No need in doing all the rest if a Cancel has been requested
            if (!frmCentral.CancelFetchInvDataInProgress) {
              Department Dept = new Department();
              if (!ret)
                ret = true;
              // Wow! Long way about getting the data below:
              //foreach (DataColumn column in tab.Columns) {
              //  if (column.ColumnName == "AccountID") {
              //    Dept.AccountID = (string)row[column];
              //  } else if (column.ColumnName == "Name") {
              //    Dept.AccountName = !row.IsNull(column) ? row[column].ToString() : String.Empty;
              //  }
              //}
              if (-1 < tab.Columns.IndexOf(ACCOUNT_ID)) {
                Dept.AccountID = row[ACCOUNT_ID].ToString();
              }
              if (-1 < tab.Columns.IndexOf(NAME)) {
                Dept.AccountName = row[NAME].ToString();
              }
              List.List.Add(Dept);
              // This statement below is logically the same as cmd.CommandText, so just don't use it
              //string dSQL = "INSERT INTO departments ( account_id, name) VALUES ('" + Dept.AccountID + "','" + Dept.AccountName + "')";
              cmd.Parameters["@account_id"].Value = Dept.AccountID;
              cmd.Parameters["@name"].Value = Dept.AccountName;
              cmd.Prepare(); // I really don't ever use this. Is it necessary? Perhaps.
              // This whole routine below is already in a Try/Catch, so this one isn't necessary
              //try {
              dbconn.DBCommand(cmd, true);
              //} catch {
              //}
            } else {
              ret = false;
              return ret;
            }
          }
        }
      }
      return ret;
    }
    

    I wrote an overload for your DBCommand method to work with Legacy code:

    public void DBCommand(SqlCeCommand cmd, string dynSQL, bool Silent) {
      cmd.CommandText = dynSQL;
      DBCommand(cmd, Silent);
    }
    
    public void DBCommand(SqlCeCommand cmd, bool Silent) {
      string dynSQL = cmd.CommandText;
      SqlCeTransaction trans = GetConnection().BeginTransaction();
      cmd.Transaction = trans;
      try {
        cmd.ExecuteNonQuery();
        trans.Commit();
      } catch (Exception ex) {
        try {
          trans.Rollback(); // I was under the impression you never needed to call this.
          // If Commit is never called, the transaction is automatically rolled back.
        } catch (SqlCeException) {
          // Handle possible exception here
        }
        MessageBox.Show("DBCommand Except 2"); // This one I haven't seen...
        //WriteDBCommandException(dynSQL, ex, Silent);
      }
    }