0

I am developing a stand alone application, using sql server compact 3.5 sp2 which runs in process. No Database writes involved. Its purely a reporting application. Read many articles about reusing open db connections in case of sql compact(connection pooling) due to its different behavior from sql server.

Quoting the comments from a quiz opened by Erik Ejlskov Jensen Link, where its discussed an open early close late strategy for sql server compact databases. Based on this, with my limited experience I have implemented a not so complex Connection handling+Data access layer. Basically I am unsure if i am writing it in a recommended way. Please could any one point me in the right direction with rooms for improvement in this connection handling approach i have written?

The DbConnection class

public class FkDbConnection
{
  private static SqlCeConnection conn; 
  private static DataTable table;
  private static SqlCeCommand cmd;
      
  ~FkDbConnection() { conn = null; }

  //This will be called when the main winform loads and connection will be open as long as the main form is open
  public static string ConnectToDatabase()
  {
     try {
      conn = new SqlCeConnection(ConfigurationManager.ConnectionStrings["Connstr"].ConnectionString);
      if (conn.State == ConnectionState.Closed || conn.State == ConnectionState.Broken)
      {
          conn.Open();
      }
      return "Connected";
     }
     catch(SqlCeException e) { return e.Message; }
  }

  public static void Disconnect()
  {
    if (conn.State == ConnectionState.Open || conn.State == ConnectionState.Connecting || conn.State == ConnectionState.Fetching)
    {
      conn.Close();
      conn.Dispose();
      //conn = null; //does conn have to be set to null?
    }
    //else the connection might be already closed due to failure in opening it
    else if (conn.State == ConnectionState.Closed) {
      conn.Dispose();
      //conn = null; //does conn have to be set to null?
    }
  }

/// <summary>
///  Generic Select DataAccess
/// </summary>
/// <param name="sql"> the sql query which needs to be executed by command object </param>
public static DataTable ExecuteSelectCommand(SqlCeCommand comm)
{
if (conn != null && conn.State == ConnectionState.Open)
            {
                #region block using datareader
                using (table = new DataTable())
                {
                    //using statement needed for reader? Its closed below
                    using (SqlCeDataReader reader = comm.ExecuteReader())
                    {
                        table.Load(reader);
                        reader.Close(); //is it needed?
                    }
                }
                #endregion
                # region block using dataadpater
                //I read DataReader is faster?
                //using (SqlCeDataAdapter sda = new SqlCeDataAdapter(cmd))
                //{
                //    using (table = new DataTable())
                //    {
                //        sda.Fill(table);
                //    }
                //}
                #endregion
            //}
            }
            return table;
        }

        /// <summary>
        ///  Get Data
        /// </summary>
        /// <param name="selectedMPs"> string csv, generated from a list of selected posts(checkboxes) from the UI, which forms the field names used in SELECT </param>
        public static DataTable GetDataPostsCars(string selectedMPs)
        {
            DataTable dt;
            //i know this it not secure sql, but will be a separate question to pass column names to select as parameters
            string sql = string.Format(
                "SELECT " + selectedMPs + " "+
                "FROM GdRateFixedPosts");
            using (cmd = new SqlCeCommand(sql,conn))
            {
                cmd.CommandType = CommandType.Text;
                //cmd.Parameters.Add("@fromDateTime",DbType.DateTime);
                //cmd.Parameters.Add("@toDateTime",DbType.DateTime);
                dt = ExecuteSelectCommand(cmd);
            }
            return dt;
        }

    }

The Main UI (Form) in which connection opened, for connection to be open through out. 2 other reporting forms are opened from here. Closing main form closes all, at which point connection is closed and disposed.

private void FrmMain_Load(object sender, EventArgs e)
{
  string str = FkDbConnection.ConnectToDatabase();
  statStDbConnection.Items[0].Text = str;
}

private void FrmMain_FormClosing(object sender, FormClosingEventArgs e)
{
    FkDbConnection.Disconnect();
}

Comments, improvements on this connection class much appreciated. See my questions also inline code Thank you.

Updated classes as per Erik's suggestion. with a correction on ExecuteSelectCommand() and an additional class which will instantiate command objs in "using" and pass data to the UI. I intent to add separate GetDataForFormX() methods since the dynamic sql for each form may differ. Hope this is ok?

Correction to Erik's code:

public static DataTable ExecuteSelectCommand(SqlCeCommand comm)
{
  var table = new DataTable();
  if (conn != null && conn.State == ConnectionState.Open)
  {
     comm.Connection = conn;
     using (SqlCeDataReader reader = comm.ExecuteReader())
     {
       table.Load(reader);
     }
  }
  return table;
}

New FkDataAccess class for passing Data to UI

public class FkDataAccess
{

  public static DataTable GetDataPostsCars(string selectedMPs)
  {
    var table = new DataTable();
    string sql = string.Format(
                "SELECT " + selectedMPs + " " +
                "FROM GdRateFixedPosts");
    if (FkDbConnection.conn != null && FkDbConnection.conn.State == ConnectionState.Open)
    {
      using (SqlCeCommand cmd = new SqlCeCommand(sql, FkDbConnection.conn))
      {
        cmd.CommandType = CommandType.Text;
       //cmd.Parameters.Add("@fromDateTime",DbType.DateTime);
        table = FkDbConnection.ExecuteSelectCommand(cmd);
      }
    }
  return table;    
  }

  //public static DataTable GetDataXY(string selectedvals)
  // and so on

}
Nish
  • 19
  • 7

1 Answers1

0

Too much code in your data access class, makes it unreadable and hard to maintain

The SqlCeonnection object will be disposed when you close it (and when the app closes)

You cannot dispose the DataTable if you want to use it elsewhere, and it is an completely managed object anyway.

It is a good pattern to limit your classes to a single responsibility

public class FkDbConnection
{
  private static SqlCeConnection conn; 

  ~FkDbConnection() { conn = null; }

  //This will be called when the main winform loads and connection will be open as long as the main form is open
  public static void ConnectToDatabase()
  {
      // Handle failure to open in the caller
      conn = new SqlCeConnection(ConfigurationManager.ConnectionStrings["Connstr"].ConnectionString);
      conn.Open();
  }

  public static void Disconnect()
  {
    if (conn != null)
    {
      conn.Close();
    }
  }

public static DataTable ExecuteSelectCommand(SqlCeCommand comm)
{
    var table = new DataTable();
    if (conn != null && conn.State == ConnectionState.Open)                
    {

       comm.Connection = conn;
       using (SqlCeDataReader reader = comm.ExecuteReader())
       {
          table.Load(reader);
       }           
    }
    return table;
}
private void FrmMain_Load(object sender, EventArgs e)
{
  try
  {
     FkDbConnection.ConnectToDatabase();
     statStDbConnection.Items[0].Text = "Connected";
  }
  catch (Exception ex)
  {
     //Inform use that we canot proceed, what she can do to remedy, and exit
  }
}

private void FrmMain_FormClosing(object sender, FormClosingEventArgs e)
{
    FkDbConnection.Disconnect();
}
ErikEJ
  • 40,951
  • 5
  • 75
  • 115
  • Wow! the Erik himself.I am honored.:-). Thank you for that invaluable review. Yeah I had sensed that the class is getting bigger. What about disposing the conn object, and the DataTable? Did you imply that it will be handled in the formclosing or in the reader dispose? Secondly, did you also imply to put the GetDataPostsCars(string selectedMPs) and likewise form based data access methods in to another class eg:DataAccess, or you overlooked it? – Nish Dec 14 '14 at 09:25
  • Hi Erik. Thanks a lot. DataTable object is managed due to the fact that its assigned to a var type? or by default it is by GAC?I have edited your reply.I corrected the ExecuteSelectCommand() function to take the instantiation and return of the DataTable object outside the If condition, else the function complains about "not having a return value". Hope its ok. i am still learning, so please excuse such basic questions. Please see edit of your last edit. – Nish Dec 14 '14 at 13:02
  • see http://stackoverflow.com/questions/913228/should-i-dispose-dataset-and-datatable + updated my reply – ErikEJ Dec 15 '14 at 10:18
  • Hi Erik. You guided me in the right direction. I will go through the content in the link, but i did get the message from a quick read. Would you also review the additional "FkDataAccess" class specifically for building the sql, command objects which passes data to the UI. I am including a using() for the command object. hope this is ok? i mean i will have separate GetDataForFormX() methods for each form in which command objs are instantiated in "using".. hope this is not a performance problem from sql compact point of view. – Nish Dec 15 '14 at 11:51