Function to run MySQL SELECT queries with no leaks

Question

I am upgrading an old massive website that is a Classic ASP \ ASP.NET hybrid. I still have a long way to MVC.

Right now I want to convert all code that uses ODBC to the mysql connector specific code.

I've made these 2 functions:

1:

DataTable GetData(string query, List<MySqlParameter> sqlParams, string sComment="")
{
    DataTable dt_tmp = new DataTable();

    if (sqlParams==null)
        sqlParams = new List<MySqlParameter>();

    Trace.Write(sComment, GenerateSQLString(query, sqlParams));
    using(MySqlDataReader myReader = MySqlHelper.ExecuteReader(sCon, query, sqlParams.ToArray()))
    {
        dt_tmp.Load(myReader, LoadOption.OverwriteChanges);
    }
    return dt_tmp;
}

2:

DataTable ReadData(string query, List<MySqlParameter> sqlParams, string sComment="")
{
    DataTable dt_tmp = new DataTable();

    if (sqlParams==null)
        sqlParams = new List<MySqlParameter>();

    Trace.Write(sComment, GenerateSQLString(query, sqlParams));

    using(MySqlConnection myConnection = new MySqlConnection(sCon))
    {
        using(MySqlCommand myCommand = myConnection.CreateCommand())
        {
            myCommand.CommandText = query;
            myCommand.Parameters.AddRange(sqlParams.ToArray());
            using (MySqlDataAdapter myAdapter = new MySqlDataAdapter(myCommand))
            {
                myAdapter.Fill(dt_tmp);
            }
        }
    }

    return dt_tmp;
}

Function #1: gets a ConstraintException (MaxLength) on a specific call... but otherwise performs very well.

Which one will perform better? (Ignoring the fact one throws an exception, which I can handle)

My goal is to have the connection open for the minimum amount of time possible so it won't use all connections from the pool.

Answer 1

Hide and seek

The first method is shorter but is that a good thing? Compared to the second one it hides a few thing internally which can be either good or bad and i would say bad. To me this method looks like a core application logic which has to be crystal clear so i think you shold not prefer the shorter version.

But wait

The second one still has some coding issues but nothing catastrophe. I have changed it in some parts, lets see:

    private DataTable ReadData(string query, string sComment = "")
    {
        return ReadData(query, EmptyMySqlParameters, sComment);
    }

    DataTable ReadData(string query, ICollection<MySqlParameter> sqlParams, string sComment = "")
    {
        var dt = new DataTable();

        Trace.Write(sComment, GenerateSQLString(query, sqlParams));

        using (var myConnection = new MySqlConnection(sCon))
        {
            myConnection.Open();

            using (var myCommand = myConnection.CreateCommand())
            {
                myCommand.CommandText = query;

                foreach (var mySqlParameter in sqlParams)
                {
                    myCommand.Parameters.Add(mySqlParameter);
                }

                using (var myReader = myCommand.ExecuteReader())
                {
                   dt.Load(myReader, LoadOption.OverwriteChanges);
                }
            }
        }

        return dt;
    }

First i have introduced an overloaded method to remove the null check against the List (null object pattern for collections are the empty versions of them). Changed the type to ICollection which is more flexible and intruduced a new static member which get involved in the process where no parameter is passed (can be static for reading it is thread safe).

I also removed the AddRange call becouse internally it is interating through the parameters with a foreach loop but only works with Array type parameter so after introducing our foreach loop calling Add manually we have removed an extra ToArray() call which would involve an unnecessary memory copy in the background.

Also changed the fill of the DataTable to a more used way and the MySqlHelper.ExecuteReader() does the same thing too.