ADO.NET - Bad Practice?

Question

I was reading an article in MSDN several months ago and have recently started using the following snippet to execute ADO.NET code, but I get the feeling it could be bad. Am I over reacting or is it perfectly acceptable?

private void Execute(Action<SqlConnection> action)
{
    SqlConnection conn = null;
    try {
        conn = new SqlConnection(ConnectionString);
        conn.Open();
        action.Invoke(conn);
    } finally {
        if (conn != null && conn.State == ConnectionState.Open) {
            try {
                conn.Close();
            } catch {
            }
        }
    }
}

public bool GetSomethingById() {
    SomeThing aSomething = null
    bool valid = false;
    Execute(conn =>
    {
        using (SqlCommand cmd = conn.CreateCommand()) {
            cmd.CommandText = ....
            ...
            SqlDataReader reader = cmd.ExecuteReader();
            ...
            aSomething = new SomeThing(Convert.ToString(reader["aDbField"]));
        }
    });
    return aSomething;
}

Answer 1

That's acceptable. I've created a SqlUtilities class two years ago that had a similar method. You can take it one step further if you like.

EDIT: Couldn't find the code, but I typed a small example (probably with many syntax errors ;))

SQLUtilities

public delegate T CreateMethod<T> (SqlDataReader reader);
public static T CreateEntity<T>(string query, CreateMethod<T> createMethod, params SqlParameter[] parameters) {
   // Open the Sql connection
   // Create a Sql command with the query/sp and parameters
   SqlDataReader reader = cmd.ExecuteReader();
   return createMethod(reader);
   // Probably some finally statements or using-closures etc. etc.
} 

Calling code

private SomeThing Create(SqlDataReader reader) {
    SomeThing something = new SomeThing();
    something.ID = Convert.ToIn32(reader["ID"]);
    ...
    return something;
}

public SomeThing GetSomeThingByID (int id) {
    return SqlUtilities.CreateEntity<SomeThing> ("something_getbyid", Create, ....);
}

Of course you could use a lambda expression instead of the Create method, and you could easily make a CreateCollection method and reuse the existing Create method.

However if this is a new project. Check out LINQ to entities. Is far easier and flexible than ADO.Net.

Answer 2

Not necessarily if that's all you need to do. However it doesn't scale well, so that's why we have frameworks such NHibernate, EF, etc.

Answer 3

i like this pattern! it is something what c# got from functional languages. the only thing is that you might not need to create connection in every action, but it can be changed. please explain why is it bad?

Answer 4

This is a very reasonable approach to use.

By wrapping your connection logic into a method which takes an Action<SqlConnection>, you're helping prevent duplicated code and the potential for introduced error. Since we can now use lambdas, this becomes an easy, safe way to handle this situation.

Answer 5

IMHO it is indeed a bad practice, since you're creating and opening a new database-connection for every statement that you execute.

Why is it bad:

• performance wise (although connection pooling helps decrease the performance hit): you should open your connection, execute the statements that have to be executed, and close the connection when you don't know when the next statement will be executed.

• but certainly context-wise. I mean: how will you handle transactions ? Where are your transaction boundaries ? Your application-layer knows when a transaction has to be started and committed, but you're unable to span multiple statements into the same sql-transaction with this way of working.

Answer 6

What is the point of doing that when you can do this?

public SomeThing GetSomethingById(int id) 
{
    using (var con = new SqlConnection(ConnectionString)) 
    {
        con.Open();
        using (var cmd = con.CreateCommand()) 
        {
            // prepare command
            using (var rdr = cmd.ExecuteReader()) 
            {
                // read fields
                return new SomeThing(data);
            }
        } 
    }
}

You can promote code reuse by doing something like this.

public static void ExecuteToReader(string connectionString, string commandText, IEnumerable<KeyValuePair<string, object>> parameters, Action<IDataReader> action) 
{
    using (var con = new SqlConnection(connectionString)) 
    {
        con.Open();
        using (var cmd = con.CreateCommand()) 
        {
            cmd.CommandText = commandText;
            foreach (var pair in parameters) 
            {
                var parameter = cmd.CreateParameter();
                parameter.ParameterName = pair.Key; 
                parameter.Value = pair.Value; 
                cmd.Parameters.Add(parameter);
            }
            using (var rdr = cmd.ExecuteReader()) 
            {
                action(rdr);
            }
        } 
    }    
}

You could use it like this:

//At the top create an alias
using DbParams = Dictionary<string, object>;

ExecuteToReader(
    connectionString, 
    commandText, 
    new DbParams() { { "key1", 1 }, { "key2", 2 } }),
    reader => 
    {
        // ...
        // No need to dispose
    }
)

Answer 7

Well, In my opinion check what you do before going through it.Something that is working doesn't mean it is best and good programming practice.Check out and find a concrete example and benefit of using it.But if you are considering using for big projects it would be nice using frameworks like NHibernate.Because there are a lot projects even frameworks developed based on it,like http://www.cuyahoga-project.org/.