Should you reuse SqlConnection, SqlDataAdapter, and SqlCommand objects?

Question

I'm working with a DAL object that is written in a layout similar to the following code. I simplified a lot of the code code just to show the setup.

public class UserDatabase : IDisposable
{
    private SqlDataAdapter UserDbAdapter;
    private SqlCommand UserSelectCommand;
    private SqlCommand UserInsertCommand;
    private SqlCommand UserUpdateCommand;
    private SqlCommand UserDeleteCommand;

    private System.Data.SqlClient.SqlConnection SQLConnection; 

    public UserDatabase()
    {
        this.SQLConnection = new System.Data.SqlClient.SqlConnection(ConnectionString);
        this.UserDbAdapter= new SqlDataAdapter(); 
        this.UserDbAdapter.DeleteCommand = this.UserDeleteCommand;
        this.UserDbAdapter.InsertCommand = this.UserInsertCommand;
        this.UserDbAdapter.SelectCommand = this.UserSelectCommand;
        this.UserDbAdapter.UpdateCommand = this.UserUpdateCommand;
    }

    private bool FillUsers(DataSet UserDataSet, out int numberOfRecords)
    {
        bool success = true;

        numberOfRecords = 0;
        string errorMsg = null;

        this.UserDbAdapter.SelectCommand = this.GetUsersSelectCommand();

        numberOfRecords = UserDbAdapter.Fill(UserDataSet, UsersTableName);

        return success;
    }

    private SqlCommand GetUserSelectCommand()
    {
        if (this.UserSelectCommand==null)
            this.UserSelectCommand= new System.Data.SqlClient.SqlCommand();
        this.UserSelectCommand.CommandText = "dbo.Users_Select";
        this.UserSelectCommand.CommandType = System.Data.CommandType.StoredProcedure;
        this.UserSelectCommand.Connection = this.SQLConnection;
        this.UserSelectCommand.Parameters.Clear();
        this.UserSelectCommand.Parameters.AddRange(new System.Data.SqlClient.SqlParameter[] {
        new System.Data.SqlClient.SqlParameter("@RETURN_VALUE", System.Data.SqlDbType.Variant, 0, System.Data.ParameterDirection.ReturnValue, false, ((byte)(0)), ((byte)(0)), "", System.Data.DataRowVersion.Current, null)});

        return UserSelectCommand;
    }

There are multiple other Fill type functions that are written the same way reusing the Connection object, SqlCommands, and SqlDataAdapter. The SqlDataAdapter manages opening and closing of the SqlConnection internally.

So my question is multipart. Is this design bad? If so, why?

If it is bad, should it be changed to keeping things in a more local scope like the following:

    public bool FillUsers(DataSet UserDataSet)
    {
        using (SqlConnection conn = new SqlConnection(ConnectionString))
        {
            using (SqlCommand command = GetUsersSelectCommand())
            {
                using (SqlDataAdapter adapter = new SqlDataAdapter(command, conn))
                {
                    adapter.Fill(UserDataSet, UsersTableName);
                }
            }
        }
    }

This would have to be done for all the functions which seems like creating, disposing, and then remaking would be worse than keeping the items around. However this seems to be the setup I see everywhere online.

Schoonmaker

Did you measure a performance problem such that you feel the need to optimise? Database connections are pooled by design. No need to "repool" over the top. – Garett

A similar question I asked a few years back: #226627 – Garett

No performance issues linked back. I'm starting a new project and need a data access object and was curious if this was "correct" or if there was a better way. – Schoonmaker

Answer 1

No, there isn't anything wrong with that. You should dispose your objects that implement IDisposable as soon as you are done with them.

Given a SqlConnection, when you dispose of the connection, the underlying connection will simply be returned to the pool. It's not necessarily "closed" as you might think. It's best to let the connection pool do it's job. Here is a link on MSDN to ADO.NET connection pooling. Trying to make it do things it wasn't designed for (some people call this optimizing, surprisingly) is usually a trip down the rabbit hole.

Also, make sure you have actually measured and observed a problem before trying to optimize it. (and I don't mean this in a harsh way, only to save you time).

Unearthly

The top code doesn't dispose of any of the objects until the UserDatabase object is disposed of. So seems like reusing them would go against this design practice? – Schoonmaker

@Equixor: I was referring more specifically to the SqlConnection. Your class name UserDatabase is slightly misleading, because it is not an actual database. Perhaps you could make a method named GetUsers(), that would return a list of users. Your second code post is the correct one. Keeping them around is worse (and sometimes leads to hard to reproduce bugs) – Unearthly

Ok thanks. I'm sure the overall design of this could also be improved. Most of the DAL classes we have are like this and fill datasets that are then used by the BLL. – Schoonmaker