locked
Are All The Database Open/Close Necessary? RRS feed

  • Question

  • I feel this is a bit overkill opening/closing the connection so many times.  Their must be a way to simplify this but I am to much of a novice to know how, can a more experienced one assist?

    public static void DeleteCreateWrite()
    {
    	OleDbConnection oleconn = new OleDbConnection(AccessConnectionString);
    	oleconn.Open();
    	try
    	{
    		string[] tablename = new string[1] { "tbl_Data", };
    		for (int q = tablename.GetLowerBound(0); q <= tablename.GetUpperBound(0); q++)
    		{
    			OleDbCommand cmd = new OleDbCommand("DROP TABLE " + tablename[q], oleconn);
    			try { cmd.ExecuteNonQuery(); }
    			catch { }
    		}
    	}
    	catch { }
    	oleconn.Close();
    	OleDbConnection olc = new OleDbConnection(AccessConnectionString);
    	olc.Open();
    	try
    	{
    		OleDbCommand command = new OleDbCommand();
    		try
    		{
    			command.Connection = olc;
    			command.CommandText = CreateTableSQL;
    			command.CommandType = CommandType.Text;
    			try { command.ExecuteNonQuery(); }
    			catch { }
    			command.Dispose();
    		}
    		catch { }
    	}
    	catch { }
    	olc.Close();
    	OleDbConnection con1 = new OleDbConnection(AccessConnectionString);
    	con1.Open();
    	try
    	{
    		OleDbCommand cm = new OleDbCommand();
    		cm.CommandText = "Insert Into " + AccessTableName + "([C], [E], [R]) VALUES (@C, @E, @R)";
    		cm.Parameters.AddWithValue("@C", field1);
    		cm.Parameters.AddWithValue("@E", PgName);
    		cm.Parameters.AddWithValue("@R", field2);
    		cm.CommandType = CommandType.Text;
    		cm.Connection = con1;
    		try { cm.ExecuteNonQuery(); }
    		catch { }
    	}
    	catch { }
    	con1.Close();
    }

    Thursday, April 30, 2015 12:51 PM

Answers


  • Michael (@CoolDadTx) I wouldn't keep recreating the command, I don't see any reason to do that. How about this instead:

    using (var oleconn = new OleDbConnection(AccessConnectionString))
    {
        using (var command = new OleDbCommand())
        {
            command.Connection = oleconn;
            oleconn.Open();
    
            var tablename = new string[] { "tbl_Data", };
    
            //This is inefficient, combine all the drops into a single batch statement
            for (int q = 0; q <= tablename.Length; q++)
            {
                command.CommandText = "DROP TABLE " + tablename[q];
                cmd.ExecuteNonQueryIgnoringErrors();
            };
    
            // Create table SQL
            command.CommandText = CreateTableSQL;
            command.ExecuteNonQueryIgnoringErrors();
    
            // Insert into Access table
            command.CommandText = "Insert Into " + AccessTableName + "([C], [E], [R]) VALUES (@C, @E, @R)";
            command.Parameters.AddWithValue("@C", field1);
            command.Parameters.AddWithValue("@E", PgName);
            command.Parameters.AddWithValue("@R", field2);
            command.ExecuteNonQueryIgnoringErrors();
        }
    }
    


    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    • Proposed as answer by Kristin Xie Friday, May 1, 2015 7:50 AM
    • Marked as answer by Kristin Xie Thursday, May 7, 2015 11:48 AM
    Thursday, April 30, 2015 3:32 PM

All replies

  • Hi.

    You should use the Finally statement and put into all the close method calls and implement an error handling to your code.

    Please take into consideration the use of using to automatically close and dispose of objects.


    Regards,
    Bubu
    http://zsvipullo.blogspot.it

    Please mark my answer if it helped you, I would greatly appreciate it.

    Thursday, April 30, 2015 1:00 PM
  • Usually there's no reason to keep opening connections, just use the existing connection to execute all those commands.

    That said, for some reason you have decided that you want to ignore all exceptions and continue. It is possible, at least in theory, that after the execution of a command the connection is left in a bad state and cannot be re-used. What exactly do you hope to get by ignoring all exceptions I don't know, usually it's a bad idea.

    Some side notes:

    • db commands are disposable so they should be disposed too, you do this only in one case out of 3
    • the for loop that uses GetLowerBound/GetUpperBound on an array is nonsense, just use foreach
    • while it may make sense to handle exceptions thrown by ExecuteNonQuery you also have try/catch blocks for code that can't possibly throw an exception in any normal circumstances. For example the entire for loop is in a try/catch that doesn't make sense.


    • Edited by Mike Danes Thursday, April 30, 2015 1:22 PM
    Thursday, April 30, 2015 1:21 PM
  • try-finally is a lot of code if the object is disposable.

    //Your code isn't exception safe.  You are silently ignoring exceptions but your late code blocks assume
    //that it doesn't (i.e. a drop fails but you try to create anyway)
    public static void DeleteCreateWrite ()
    {
        using (var oleconn = new OleDbConnection(AccessConnectionString))
        {
            oleconn.Open();
    
            var tablename = new string[] { "tbl_Data", };
    
            //This is inefficient, combine all the drops into a single batch statement
            for (int q = 0; q <= tablename.Length; q++)
            {
                using (var cmd = new OleDbCommand("DROP TABLE " + tablename[q], oleconn))
                {
                    cmd.ExecuteNonQueryIgnoringErrors();
                };
            };
    
            using (var command = new OleDbCommand())
            {
                command.Connection = oleconn;
                command.CommandText = CreateTableSQL;
                command.CommandType = CommandType.Text;
    
                command.ExecuteNonQueryIgnoringErrors();
            };
    
            using (var cm = new OleDbCommand())
            {
                cm.CommandText = "Insert Into " + AccessTableName + "([C], [E], [R]) VALUES (@C, @E, @R)";
                cm.Parameters.AddWithValue("@C", field1);
                cm.Parameters.AddWithValue("@E", PgName);
                cm.Parameters.AddWithValue("@R", field2);
                cm.CommandType = CommandType.Text;
                cm.Connection = oleconn;
    
                cm.ExecuteNonQueryIgnoringErrors();
            };
        }
    }

    And the helper method that I don't agree with but mimics your existing code.

    static class CommandExtensions
    {
        public static void ExecuteNonQueryIgnoringErrors ( this IDbCommand command )
        {
            try
            {
                command.ExecuteNonQuery();
            } catch
            {
                /* This is bad since some exceptions aren't recoverable... */
            };
        }
    }

    Michael Taylor
    http://blogs.msmvps.com/p3net

    Thursday, April 30, 2015 3:14 PM
  • I agree with Michael, but I would stress his point further.

    You should not be using try catch all over the place like that.

    The only place you should have try catch is when you can get an expected error and you have a plan to handle it.

    Here  you insert a record... it errors... the user is completely unaware that failed.

    They work for hours on stuff.
    Come back the next day.

    None of their data is there.

    Use a global error handler.

    Plan on stopping the application if it errors.

    Otherwise you're carrying on in an unknown state. 

    Nasty things will happen.


    Hope that helps.

    Technet articles: Uneventful MVVM; All my Technet Articles

    Thursday, April 30, 2015 3:27 PM

  • Michael (@CoolDadTx) I wouldn't keep recreating the command, I don't see any reason to do that. How about this instead:

    using (var oleconn = new OleDbConnection(AccessConnectionString))
    {
        using (var command = new OleDbCommand())
        {
            command.Connection = oleconn;
            oleconn.Open();
    
            var tablename = new string[] { "tbl_Data", };
    
            //This is inefficient, combine all the drops into a single batch statement
            for (int q = 0; q <= tablename.Length; q++)
            {
                command.CommandText = "DROP TABLE " + tablename[q];
                cmd.ExecuteNonQueryIgnoringErrors();
            };
    
            // Create table SQL
            command.CommandText = CreateTableSQL;
            command.ExecuteNonQueryIgnoringErrors();
    
            // Insert into Access table
            command.CommandText = "Insert Into " + AccessTableName + "([C], [E], [R]) VALUES (@C, @E, @R)";
            command.Parameters.AddWithValue("@C", field1);
            command.Parameters.AddWithValue("@E", PgName);
            command.Parameters.AddWithValue("@R", field2);
            command.ExecuteNonQueryIgnoringErrors();
        }
    }
    


    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    • Proposed as answer by Kristin Xie Friday, May 1, 2015 7:50 AM
    • Marked as answer by Kristin Xie Thursday, May 7, 2015 11:48 AM
    Thursday, April 30, 2015 3:32 PM
  • The overhead of creating a command isn't high so personal I wouldn't worry about it.  The only command I'd consider reusing is the one in the foreach loop as it is the same command.  The problem with reusing commands in the above code is that if someone comes along and adds another set of inserts later they have to remember to first remove the existing parameters. 

    Personally I would use a single command and batch each drop/create/insert into a single set of statements.  Then a single foreach loop for each table would work.

    Thursday, April 30, 2015 3:45 PM
  • Personally I would use a single command and batch each drop/create/insert into a single set of statements.  Then a single foreach loop for each table would work.


    It doesn't look like he is creating and inserting into multiple tables, like he's doing with the drop ... so a single foreach probably isn't going to work for his purposes. But, if that *was* what he was doing, then I'd agree with you!

    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    Thursday, April 30, 2015 3:57 PM
  • Using the below I get an error of the index was outside the bounds of the array?
    for (int q = 0; q <= tablename.Length; q++)
    Thursday, April 30, 2015 9:22 PM
  • Using the below I get an error of the index was outside the bounds of the array?
    for (int q = 0; q <= tablename.Length; q++)
    Change "<=" to only "<".

    Armin

    Thursday, April 30, 2015 9:42 PM