Is it OK to pass SQLCommand as a parameter?

Question

I have a Business Layer that passes a Conn string and a SQLCommand to a Data Layer like so

    public void PopulateLocalData()
    {
       System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
       cmd.CommandType = System.Data.CommandType.StoredProcedure;
       cmd.CommandText = "usp_PopulateServiceSurveyLocal";
       DataLayer.DataProvider.ExecSQL(ConnString, cmd);
    }

The DataLayer then just executes the sql like so

        public static int ExecSQL(string sqlConnString, System.Data.SqlClient.SqlCommand cmd)
    { 
        int rowsAffected;
        using (SqlConnection conn = new SqlConnection(sqlConnString))
        {
            conn.Open();
            cmd.Connection = conn;
            rowsAffected = cmd.ExecuteNonQuery();
            cmd.Dispose();
        }
        return rowsAffected;
    }

Is it OK for me to pass the SQLCommand as a parameter like this or is there a better more accepted way of doing it. One of my concerns is if an error occurs when executing the query the cmd.dispose line will never execute. Does that mean it will continue to use up memory that will never be released?

Update:

Following Eric's advice I more explicitly divided the Business and Data Layers so the method in the Business Layer looks like this

    public void PopulateLocalData()
    {
        DataLayer Data = new DataLayer(this.ConnString);
        Data.UpdateLocalData();
    }

and method that is called in the DataLayer looks like this.

        public void UpdateLocalData()
    {
        using (SqlConnection conn = new SqlConnection(this.ConnString))
        using(SqlCommand cmd = new SqlCommand())
        {
            cmd.CommandType = System.Data.CommandType.StoredProcedure;
            cmd.CommandText = "usp_PopulateServiceSurveyLocal";
            conn.Open();
            cmd.Connection = conn;
            cmd.ExecuteNonQuery();
        }
    }

This way it is very clear that both the SQLCommand and the SQLConnection will be disposed of properly. Thanks.

Answer 1

Well for starters, you could change it to:

public static int ExecSQL(string sqlConnString, System.Data.SqlClient.SqlCommand cmd)
{ 
    int rowsAffected;
    try
    {
        using (SqlConnection conn = new SqlConnection(sqlConnString))
        {
            conn.Open();
            cmd.Connection = conn;
            rowsAffected = cmd.ExecuteNonQuery();
        }
    } finally {
        cmd.Dispose();
    }
    return rowsAffected;
}

Additionally, I generally separate my business and data layers more than you do. My business layer would call a method "GetLocalSurvey" in the data layer, which would handle all of the SQL nonsense.

Answer 2

Why don't you change it to this:

public static int ExecProcedure(string sqlConnString, string procedureName)
{
    using (var cmd = new System.Data.SqlClient.SqlCommand())
    {
        cmd.CommandType = System.Data.CommandType.StoredProcedure;
        cmd.CommandText = procedureName;
        int rowsAffected;
        using (SqlConnection conn = new SqlConnection(sqlConnString))
        {
            conn.Open();
            cmd.Connection = conn;
            return cmd.ExecuteNonQuery();
        }
    }
}

Do you want additional parameters? Create overloads, refactor. Share the most code in common function. Creating new System.Data.SqlClient.SqlCommand() everywhere is wrong approach.

Answer 3

Ideally, your business layer should not be aware of the implementation details of your data layer. So, whether you implement the data layer with SqlCommand objects or with something like NHibernate, should be irrelevant to the business layer. This makes it theoretically speaking easy to 'shift out' your data layer and replace it with another one.

Summarizing: passing the SqlCommand from the business layer to the data layer is in my eyes not considered good practice.

Regarding Dispose(): if you are using a using-statement (like using(SqlConnection ...)), the Dispose() method is called automatically at the end of the using statement. You don't have to do this manually.

Answer 4

The one creating the Command should be responsible for disposing it. The easiest way for this is to remove the call to cmd.Dispose from ExecSql and instead call your function like this:

public void PopulateLocalData() 
{ 
   using (System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand())
   {
       cmd.CommandType = System.Data.CommandType.StoredProcedure; 
       cmd.CommandText = "usp_PopulateServiceSurveyLocal"; 
       DataLayer.DataProvider.ExecSQL(ConnString, cmd);
   }
} 

One of my concerns is if an error occurs when executing the query the cmd.dispose line will never execute. Does that mean it will continue to use up memory that will never be released?

Coincidentally, SqlClient.SqlCommand does not need to be disposed. This, however, is an implementation detail that you should not rely on -- the general rule is still: If it implements IDisposable, dispose it.(SqlCeClient.SqlCeCommand, for example, does need to be disposed...)