Why did I only get part of what I intend to get?

Why did I only get part of what I intend to get?

• Jamie Thomson

  This problem is solved now.

  As Taylor said, exeSql method of DBClass resulted in the error, because it opened a new connection to DB each time the SQL command was executed thus ate up all the available DB connection.

  Now I have changed the code as Taylor said, it works smoothly now.

  Thank you all who has helped me in this problem in various ways. All of your generous help has done me much good.

  Thank you all again!

  
  
  

• Özge Çolak

  Anthony ,

  Thank you for your advice, your style is indeed a good habit for programmer. I will keep to it in the future.

  Thks again for your kindness!

  
  
  

• Mohamed Sami

  Thanks.

  Now I rewrite the InsertAMonth code by putting db.clear() in the finally{ } brackets.

  Now I got a complaint:

  System.InvalidOperationException: Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached. at System.Data.SqlClient.SqlConnectionPoolManager.GetPooledConnection(SqlConnectionString options, Boolean& isInTransaction) at System.Data.SqlClient.SqlConnection.Open() at database.DBClass.connDb() at database.DBClass.exeSql() at calendar.calendar.InsertAMonth(Int32 x)
  

  
  
  

• VS2005_evaluator

  The problem lies in your connDb method. It opens a new SQL connection each time it is called irrelevant of whether the connection is already open or not. This method is called by execSql. Therefore each time through your for loop you are creating another connection to the DB.

  I would recommend that you modify connDb to only create the connection if conn is null. This means it'll create the connection the very first time execSql is called but reuse the same connection on subsequent calls.

  private void connDb()
  {
  if (conn != null)
  return
  
  conn = new SqlConnection(connStr);
  try
  {
  conn.Open();
  }
  catch(SqlException e)
  {
  for(int i=0;i<e.Errors.Count;i++)
  {
  errInfo+="Error Sequential Number:" +i+ "\n" + "Error Info:" +e.Errors.Message+ "\n" + "Error Source:" +e.Errors.Source+ "\n" + "Program:" +e.Errors.Procedure;
  }
  conn.Close();
  }
  
  }

  Your Clear method may throw an exception if conn is not set so you should change the code as follows.

  public void clear() // clear method should be able to ensure a connection be closed once InsertAMonth is completed
  {
  if (conn != null)
  {
  conn.Close();
  conn = null;
  };
  comm = null;
  dr = null;
  ds = null;
  dad = null;
  dv = null;
  }
  
  To simplify your code and make it more .NET compliant I would recommend that you implement IDisposable in your database class like so:
  
  public class database : IDisposable
  {
  ~database()
  {
  Dispose(false);
  }
  
  void IDisposable.Dispose()
  { Close(); }
  
  public void Close( )
  {
  Dispose(true);
  GC.SuppressFinalize(this);
  }
  
  private void Dispose ( bool disposing )
  {
  if (disposing)
  {
  //Can touch reference type fields
  if (conn != null)
  {
  conn.Close();
  conn = null;
  };
  
  //Additional cleanup
  };
  }
  }

  You can then simply your main logic like so:
  
  

  private bool InsertAMonth(int x)
  {
  using (DBClass db = new DBClass())
  {
  try
  {
  db.ConnStr = ConfigurationSettings.AppSettings["cstr_calendar"];
  for (int i = 1; i<=x; i++)
  {
  _ri = i;
  db.Sql = "insert into solar (nian,yue,ri,xingqi) values ('"+_nian+"','"+_yue+"','"+_ri+"','"+_xingqi+"')";
  db.exeSql();
  if (_xingqi < 7)
  {_xingqi += 1;}
  else
  {_xingqi = 1;}
  }
  return true;
  } catch (System.Exception E)
  {
  _errorInfo = E.ToString();
  return false;
  };
  };
  }

  The runtime will then guarantee that your object is disposed properly. More importantly each time this method is called you will open at most one connection to the DB and it'll be closed when the method finishes irrelevant of exceptions.

  Michael Taylor - 7/28/06

  
  

• Ed Dixon

  Your programming logic works correctly. I converted the database calls to a simple dump to the console and all the values got there correctly.

  Therefore if not all the data is getting into the DB then I'd assume that the problem lies in the database class or the database itself. Step through your code (perhaps set a breakpoint in your catch block) and verify that each value is being written properly to the DB.

  One thing that does stand out is your connection management code. You open a connection for each month. If this connection is not closed then you'll quickly eat up all available connections and will eventually start failing to connect to the DB. Furthermore if an exception occurs your db.Clean() method will never be called so if the connection is closed here then an exception will leave the DB connection open.

  Michael Taylor - 7/28/06

  
  

• Higgs Boson

  But I changed connDB as follows, the error became

  System.InvalidOperationException: ExecuteNonQuery: Connection property has not been initialized. at System.Data.SqlClient.SqlCommand.ValidateCommand(String method, Boolean executing) at System.Data.SqlClient.SqlCommand.ExecuteNonQuery() at database.DBClass.exeSql() at calendar.calendar.InsertAMonth(Int32 x)

  private void connDb()
  {
  if (conn != null)
  {
  conn = new SqlConnection(connStr);

  try
  {
  conn.Open();
  }
  catch(SqlException e)
  {
  for(int i=0;i<e.Errors.Count;i++)
  {
  errInfo+="Error Sequential Number:" +i+ "\n" + "Error Info:" +e.Errors.Message+ "\n" + "Error Source:" +e.Errors.Source+ "\n" + "Program:" +e.Errors.Procedure;
  }
  conn.Close();
  }
  }
  }

  
  
  

• MacDara

  Tom, as a matter of style and readability use String.Format rather than the concatenation of strings "('" + _nian + "','" + ...

  Instead use:

  String.Format(

  "INSERT INTO solar (nian,yue,ri,xingqi) VALUES ('{0}', '{1}', '{2}', '{3}')",

  nian, yue, ri, xingq);

  There's no need to call into the fields directly as the JiT compiler will just inline them (most likely) but if you prefer you can still use the fields.

  Another place you could improve your code. Instead of the switch block with:

  case 1: _yue = 1; InsertAMonth(31); break;
  ...
  case 12: _yue = 12; InsertAMonth(31); break;

  you could more concisely write:

  int[] monthLengths = {0,
  31, 28, 31,
  30, 31, 30,
  31, 31, 30,
  31, 30, 31}; //Should be a class level shared variable.

  for (int j = 1; j<=12; j++)
  {
  _yue = j; InsertAMonth(monthLengths[j]);
  }

  When you say you only get some of the year's data. Which months Is it just the beginning Just the end Totally random I think that if you refactor your code significantly you'll be able to debug easier.
  
  

  
  
  

• ksona

  The problem is that you are opening a new connection to the DB but never closing the connection. You need to look at your database class. It should expose a method to close the underlying database connection. If it doesn't then you'll run out of connections and get the error you mentioned. If the Clear method is closing the DB connection then look at your execSql method. Does it open a connection to the DB as well If so then you're leaking connection objects. You need to make sure you close this connection after each iteration of your for loop. Ideally execSql should look to see if the connection is already open. If so then it should use the connection otherwise it should open the connection. This would allow you to open the connection once and reuse it in subsequent loops. Then you only need to close the connection after the loop completes (in your finally block).

  Michael Taylor - 7/28/06

  
  

• DawnJ

  Mr. Michael Taylor:

  How professional you are! Thank you very, very much!

  You do me a great favor, your help is sincerely appreciated!

  Thanks again!

  
  
  

• eldiener

  I am too sleepy now, tomorrow I will come here to check myself first, my head is not clear right now.

  :) thks!

  
  
  

• 2lazydba

  Thks a million!

  I already changed InsertAMonth code as this, but the error still exists:

  private bool InsertAMonth(int x)
  {
  DBClass db = new DBClass();
  try
  {
  db.ConnStr = ConfigurationSettings.AppSettings["cstr_calendar"];
  for (int i = 1; i<=x; i++)
  {
  _ri = i;
  db.Sql = "insert into solar (nian,yue,ri,xingqi) values ('"+_nian+"','"+_yue+"','"+_ri+"','"+_xingqi+"')";
  db.exeSql();
  if (_xingqi < 7)
  {_xingqi += 1;}
  else
  {_xingqi = 1;}
  }
  return true;
  }
  catch (System.Exception E)
  {
  _errorInfo = E.ToString();
  return false;
  }
  finally
  {
  db.clear(); // Here I put db.clear in finally bracket, so it will surely be executed, right
  db = null;
  }
  }

  database.dll is as follows:

  using System;
  using System.Data;
  using System.Data.SqlClient;

  namespace database
  {
  public class DBClass
  {
  private SqlConnection conn;
  private SqlCommand comm;
  public SqlDataReader dr;
  public DataSet ds;
  public SqlDataAdapter dad;
  public DataView dv;
  private string sql;
  private string connStr;
  private string errInfo;
  
  public void database()
  {
  errInfo = "";
  }
  
  public string ErrInfo
  {
  get {return errInfo;}
  }
  public string Sql
  {
  get {return sql;}
  set {sql=value;}
  }
  public string ConnStr
  {
  get {return connStr;}
  set {connStr=value;}
  }
  
  private void connDb()
  {
  conn = new SqlConnection(connStr);
  try
  {
  conn.Open();
  }
  catch(SqlException e)
  {
  for(int i=0;i<e.Errors.Count;i++)
  {
  errInfo+="Error Sequential Number:" +i+ "\n" + "Error Info:" +e.Errors.Message+ "\n" + "Error Source:" +e.Errors.Source+ "\n" + "Program:" +e.Errors.Procedure;
  }
  conn.Close();
  }
  
  }
  
  public void dataView()
  {
  connDb();
  dad = new SqlDataAdapter(sql,conn);
  ds = new DataSet();
  dad.Fill(ds,"article");
  dv = new DataView(ds.Tables["article"]);
  }
  
  public void readerData()
  {
  connDb();
  comm = new SqlCommand(sql,conn);
  dr = comm.ExecuteReader();
  }
  
  public void exeSql()
  {
  connDb();
  comm = new SqlCommand(sql,conn);
  comm.ExecuteNonQuery();
  }
  
  public void clear() // clear method should be able to ensure a connection be closed once InsertAMonth is completed
  {
  conn.Close();
  comm = null;
  dr = null;
  ds = null;
  dad = null;
  dv = null;
  }
  }

  }

  
  

  
  
  
• Why did I only get part of what I intend to get?