An enhanced ExecuteReader method in VB.NET

Question

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.

• jmoreno: 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.

Answer 1

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.