2
\$\begingroup\$

The idea was creating and preparing a SqlCommand, and call ExecuteReader with the appropriate command behavior. If we created and opened the connection, we want the connection to be closed when the DataReader is closed. If the caller provided the connection, we want to leave it to them to manage.

  • connection - a valid SqlConnection, on which to execute this command
  • transaction - a valid SqlTransaction, or ' null'
  • commandType - the CommandType (stored procedure, text, etc.)
  • commandText - the stored procedure name or T-SQL command
  • commandParameters - an array of SqlParameters to be associated with the command or ' null' if no parameters are required
  • connectionOwnership - indicates whether the connection parameter was provided by the caller, or created by SqlHelper

Returns: SqlDataReader containing the results of the command

I am thinking that it is like reinventing the wheel.

    Private Overloads Shared Function ExecuteReader(ByVal connection As SqlConnection, _
                                                    ByVal transaction As SqlTransaction, _
                                                    ByVal commandType As CommandType, _
                                                    ByVal commandText As String, _
                                                    ByVal commandParameters() As SqlParameter, _
                                                    ByVal connectionOwnership As SqlConnectionOwnership) As SqlDataReader

        If (connection Is Nothing) Then Throw New ArgumentNullException("connection")

        Dim mustCloseConnection As Boolean = False
        ' Create a command and prepare it for execution
        Dim cmd As New SqlCommand
        Try
            ' Create a reader
            Dim dataReader As SqlDataReader

            PrepareCommand(cmd, connection, transaction, commandType, commandText, commandParameters, mustCloseConnection)

            ' Call ExecuteReader with the appropriate CommandBehavior
            If connectionOwnership = SqlConnectionOwnership.External Then
                dataReader = cmd.ExecuteReader()
            Else
                dataReader = cmd.ExecuteReader(CommandBehavior.CloseConnection)
            End If

            ' Detach the SqlParameters from the command object, so they can be used again
            Dim canClear As Boolean = True
            Dim commandParameter As SqlParameter
            For Each commandParameter In cmd.Parameters
                If commandParameter.Direction <> ParameterDirection.Input Then
                    canClear = False
                End If
            Next

            If (canClear) Then cmd.Parameters.Clear()

            Return dataReader
        Catch
            If (mustCloseConnection) Then connection.Close()
            Throw
        End Try
    End Function

For the PrepareCommand method.

    Private Shared Sub PrepareCommand(ByVal command As SqlCommand, _
                                      ByVal connection As SqlConnection, _
                                      ByVal transaction As SqlTransaction, _
                                      ByVal commandType As CommandType, _
                                      ByVal commandText As String, _
                                      ByVal commandParameters() As SqlParameter, ByRef mustCloseConnection As Boolean)

        If (command Is Nothing) Then Throw New ArgumentNullException("command")
        If (commandText Is Nothing OrElse commandText.Length = 0) Then Throw New ArgumentNullException("commandText")

        ' If the provided connection is not open, we will open it
        If connection.State <> ConnectionState.Open Then
            connection.Open()
            mustCloseConnection = True
        Else
            mustCloseConnection = False
        End If

        ' Associate the connection with the command
        command.Connection = connection

        ' Set the command text (stored procedure name or SQL statement)
        command.CommandText = commandText

        ' If we were provided a transaction, assign it.
        If Not (transaction Is Nothing) Then
            If transaction.Connection Is Nothing Then Throw New ArgumentException("The transaction was rollbacked or commited, please provide an open transaction.", "transaction")
            command.Transaction = transaction
        End If

        ' Set the command type
        command.CommandType = commandType

        ' Attach the command parameters if they are provided
        If Not (commandParameters Is Nothing) Then
            AttachParameters(command, commandParameters)
        End If
        Return
    End Sub

Code for AttachParameters

    Private Shared Sub AttachParameters(ByVal command As SqlCommand, ByVal commandParameters() As SqlParameter)
        If (command Is Nothing) Then Throw New ArgumentNullException("command")
        If (Not commandParameters Is Nothing) Then
            Dim p As SqlParameter
            For Each p In commandParameters
                If (Not p Is Nothing) Then
                    ' Check for derived output value with no value assigned
                    If (p.Direction = ParameterDirection.InputOutput OrElse p.Direction = ParameterDirection.Input) AndAlso p.Value Is Nothing Then
                        p.Value = DBNull.Value
                    End If
                    command.Parameters.Add(p)
                End If
            Next p
        End If
    End Sub

Please give me any advice on enhancing this code.

\$\endgroup\$
1
  • \$\begingroup\$ ByVal is useless and should be removed to make the code easier to read. IsNot was added quite a few versions ago and reads better. It’s a bit more work you can make it use the base DB classes and not the specific SQLclient classes, which would allow usage of things like glimpse/miniprofiler. \$\endgroup\$ Commented Jan 5 at 3:55

1 Answer 1

Reset to default
2
\$\begingroup\$

I am thinking that it is like reinventing the wheel.

In some way, that could be true. But let's see.

Testing function arguments

I believe some design choices are questionable. First of all, your ExecuteReader function accepts a number of arguments, among which connection. So the very first thing you do is:

If (connection Is Nothing) Then Throw New ArgumentNullException("connection")

So far so good.

But then later on you call PrepareCommand:

PrepareCommand(cmd, connection, transaction, commandType, commandText, commandParameters, mustCloseConnection)

In that routine, you now check the state of the connection:

' If the provided connection is not open, we will open it
If connection.State <> ConnectionState.Open Then
    connection.Open()
    mustCloseConnection = True
Else
    mustCloseConnection = False
End If

In other words, the first check that was done is not useful, because you just checked that the connection is != Nothing but it could still be invalid. It would then be better to consolidate the connection test in one single place.

Connection handling

So if am reading this right, your code will optionally open the connection if needed, but it is still the responsibility of the developer to close it, unless an exception has occurred in ExecuteReader. I feel that there is a confusion of scope/responsibility here. So it would be more consistent to only accept a valid and open connection, and make it clear that the handling of that part is left to the developer so there is no ambiguity.

Consolidate and centralize

Likewise, the validation tests for function arguments are spread across two routines, it would be better to consolidate them in one single place.

Validation and exceptions

In terms of validation, you are simply raising exceptions that would have been raised anyway:

If (command Is Nothing) Then Throw New ArgumentNullException("command")
If (commandText Is Nothing OrElse commandText.Length = 0) Then Throw New ArgumentNullException("commandText")

You are not handling exceptions here, you are re-throwing and relabeling them. This is not useful, especially if you do this and decrease the stack trace information that could be used for debugging:

If (connection Is Nothing) Then Throw New ArgumentNullException("connection")

Class-ify

Ultimately, this kind of code could typically be implemented as a class with a few private methods, so that the subfunctions cannot be called directly, and only the public methods are exposed.

Transactions

Now, if you are implementing a simple datareader why do you need a transaction? You are not making any changes to the database, in fact you should even access it read-only mode.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.