Opening and closing SQLConnections in C#

Question

can someone tell me how to use a SQL Connection in C# correct?

Right now I am doing it like that:

//some Code here
using (var sqlConnection = DatabaseUtil.DatabaseUtil.CreateSqlConnection(connectionString)) 
{
    var cmd = new SqlCommand();

    DatabaseUtil.DatabaseUtil.InitializeSqlCommand(ref cmd, query, sqlConnection);

    sqlConnection.Open();

    using (var reader = cmd.ExecuteReader(CommandBehavior.CloseConnection))
    {
        reader.Read();

        if (reader.HasRows)
        {
            //some code here
        }

        reader.Close();
        reader.Dispose();
    }

    sqlConnection.Close();
}

DatabaseUtil is written in VB.NET. Here the Function CreateSqlConnection:

Public Function CreateSqlConnection(connectionString As String) As SqlConnection
    Dim result As SqlConnection
    result = New SqlConnection(connectionString)
    Return result
End Function

And here you can see the function InitializeSqlCommand:

Public Sub InitializeSqlCommand(ByRef cmd As SqlCommand, query As String, sqlConnection As SqlConnection)
    cmd.CommandText = query
    cmd.CommandType = CommandType.Text
    cmd.Connection = sqlConnection
End Sub

Am I doing it right? Or do you have some suggestion for improvement for me?

I will be appreciative for every tip.

Ali

Answer 1

I need to start off by saying nothing in the original post is wrong, per se, and you could continue to use that code just fine.

But... we can still do better.

There is no need for the SqlCommand argument to InitializeSqlCommand() to pass by ref. IMO this is a mistake in the VB code. ByVal is good enough here, and ByRef exposes your command object to things you might not want.

In the CreateSqlConnection() function, I tend to assume if you're creating a connection, you're going to want to open it soon, too. Plus, we can shorten the method some.

I also tend to either put my connection strings directly into my equivalent DatabaseUtil modules, or build the Module so it can load the string from a config file. I don't want to need to pass that data into the CreateSqlConnection() method every time. Put these two paragraphs together like this:

Private ReadOnly Property ConnectionString As String
    Get
         Return "connection string here"
    End Get
End Property

Public Function CreateSqlConnection() As SqlConnection
    Dim result As New SqlConnection(ConnectionString)
    result.Open()
    Return result
End Function

It's a small thing, but SqlCommand also implements IDisposble, so ideally it will also be in a using block. Really, there's nothing in the existing InitializeSqlCommand() method you can't accomplish directly with the SqlCommand constructor, as CommandType.Text is already the default. Go ahead and import the DatabaseUtil namespace, and you can put these two paragraphs together like this:

using (var sqlConnection = DatabaseUtil.CreateSqlConnection())
using (var cmd = new SqlCommand(query, sqlConnection))
{

I also worry about an InitializeSqlCommand() command function that accepts a query string, but makes no provision for query parameters. Yes, you can still add parameters later in code, but in my experience this tends to encourage the use of string concatenation for parameter data... or more accurately, fails to adequately discourage it, which amounts to the same thing. You want to make sure you don't have SQL Injection vulnerabilities in the application. If you continue to use InitializeSqlCommand(), I structure it more like this:

Public Function InitializeSqlCommand(cn As SqlConnection, query As String, ParamArray paramters() As SqlParamter) As SqlCommand
    Dim result As SqlCommand = cn.CreateCommand()
    result.CommandText = query
    If parameters IsNot Nothing AndAlso parameter.Length > 0 Then
       result.Parameters.AddRange(parameters)
    End If
    Return result
End Sub

It's not necessary to call sqlConnection.Close() if the connection was created in a using block. The same applies to the DataReader.

Finally, the typical pattern for the DataReader is not to check the HasRows property. It's usually enough to only check the results of the Read() method, and usually in a while loop.

Put it all together, including the revised VB functions, like this:

var parameters = new SqlParameter[] { }; //define parameters here
using (var sqlConnection = DatabaseUtil.CreateSqlConnection())
using (var cmd = DatabaseUtil.InitializeSqlCommand(sqlConnection, query, parameters))
using (var reader = cmd.ExecuteReader())
{
    while(reader.Read())
    {
        //some code here
    }
}