views:

688

answers:

3

I seemingly have two choices:

  1. Make my class implement IDisposable. Create my DbCommand instances as private readonly fields, and in the constructor, add the parameters that they use. Whenever I want to write to the database, bind to these parameters (reusing the same command instances), set the Connection and Transaction properties, then call ExecuteNonQuery. In the Dispose method, call Dispose on each of these fields.
  2. Each time I want to write to the database, write using(var cmd = new DbCommand("...", connection, transaction)) around the usage of the command, and add parameters and bind to them every time as well, before calling ExecuteNonQuery. I assume I don't need a new command for each query, just a new command for each time I open the database (right?).

Both of these seem somewhat inelegant and possibly incorrect.

For #1, it is annoying for my users that I this class is now IDisposable just because I have used a few DbCommands (which should be an implementation detail that they don't care about). I also am somewhat suspicious that keeping a DbCommand instance around might inadvertently lock the database or something?

For #2, it feels like I'm doing a lot of work (in terms of .NET objects) each time I want to write to the database, especially with the parameter-adding. It seems like I create the same object every time, which just feels like bad practice.

For reference, here is my current code, using #1:

using System;
using System.Net;
using System.Data.SQLite;

public class Class1 : IDisposable
{
    private readonly SQLiteCommand updateCookie = new SQLiteCommand("UPDATE moz_cookies SET value = @value, expiry = @expiry, isSecure = @isSecure, isHttpOnly = @isHttpOnly WHERE name = @name AND host = @host AND path = @path");
    public Class1()
    {
        this.updateCookie.Parameters.AddRange(new[]
          {
           new SQLiteParameter("@name"),
           new SQLiteParameter("@value"),
           new SQLiteParameter("@host"),
           new SQLiteParameter("@path"),
           new SQLiteParameter("@expiry"),
           new SQLiteParameter("@isSecure"),
           new SQLiteParameter("@isHttpOnly")
          });
    }

    private static void BindDbCommandToMozillaCookie(DbCommand command, Cookie cookie)
    {
        long expiresSeconds = (long)cookie.Expires.TotalSeconds;

        command.Parameters["@name"].Value = cookie.Name;
        command.Parameters["@value"].Value = cookie.Value;
        command.Parameters["@host"].Value = cookie.Domain;
        command.Parameters["@path"].Value = cookie.Path;
        command.Parameters["@expiry"].Value = expiresSeconds;
        command.Parameters["@isSecure"].Value = cookie.Secure;
        command.Parameters["@isHttpOnly"].Value = cookie.HttpOnly;
    }

    public void WriteCurrentCookiesToMozillaBasedBrowserSqlite(string databaseFilename)
    {
        using (SQLiteConnection connection = new SQLiteConnection("Data Source=" + databaseFilename))
        {
            connection.Open();
            using (SQLiteTransaction transaction = connection.BeginTransaction())
            {
                this.updateCookie.Connection = connection;
                this.updateCookie.Transaction = transaction;

                foreach (Cookie cookie in SomeOtherClass.GetCookieArray())
                {
                    Class1.BindDbCommandToMozillaCookie(this.updateCookie, cookie);
                    this.updateCookie.ExecuteNonQuery();
                }

                transaction.Commit();
            }
        }
    }

    #region IDisposable implementation
    protected virtual void Dispose(bool disposing)
    {
     if (!this.disposed && disposing)
     {
      this.updateCookie.Dispose();
     }
     this.disposed = true;
    }
    public void Dispose()
    {
     this.Dispose(true);
     GC.SuppressFinalize(this);
    }
    ~Class1()
    {
     this.Dispose(false);
    }

    private bool disposed;
    #endregion
}
+2  A: 

There's no single "right way" to handle your database object life cycle. It's all dependent on your application needs.

My personal preference is to keep the code as simple as possible. I tend to recreate Command and Parameter objects as needed. This lets my functions be as self contained as possible. It has greatly simplified any re factoring I've had to do.

If you're concerned about the performance hit of recreating objects you should load up a profiler and see where your bottle necks are. In applications I've built, I've found the time spent creating DbCommand objects is so inconsequential compared to the time spent executing the query that it doesn't really factor in to my apps performance.

aef123
I'm not concerned about the performance hit of recreating them; I'm concerned about how the code feels when I do so. It just seems weird that you would create an object, add some parameters to it, and then bind to those parameters every time you wanted to run a database query. But, maybe that's the way to go, after all...
Domenic
A: 

I have found with sqlserver if we reuse the command with same connection, its much faster. I plan to test for different connections and transactions but they appear to be faster.

Rohit
+1  A: 

Domenic, Disposing a SQLiteCommand simply signals its active reader to dispose it's reader and sets the parameters and connection reference to null.

You are not at risk of resource leak by caching a command as long as you properly dispose of the executed reader and close/dispose of the connection.

So, reusing a cached command and simply assigning values to the parameters is by far the most efficient implementation.

.Prepare() is a noop in SQLiteCommand so there is no benefit to be had there.

Sky Sanders
So I don't need to `Dispose()` of the command itself, just of the connection and transaction? So e.g. my example code would work without all of the `IDisposable` stuff I have?
Domenic
@domenic Yes, I would say that your class does not indicate the need to be disposable and most certainly does not need a destructor. You can remove all of the IDisposable as your class is basically a Command wrapper. `WriteCurrentCookiesToMozillaBasedBrowserSqlite` does all of the cleanup that you need to do. You can reuse the same instance of the class infinitum.
Sky Sanders