Database access class using VB.NET

I've recently been asked to make some emergency changes to a legacy application written in VB.NET. I come from a C# background so am not overly familiar with this language, though there are enough similarities for me to get by.

The current code has no separation of concerns (e.g. DAL code is written in the codebehind of each page rather than separated out). To make it usable without introducing too much risk I'm avoiding libraries but extracting the DAL code to separate classes. Since my DB code's going to be used from a lot of places in the app, I wanted to check that this all makes sense, that it is reasonably efficient and that I haven't made some schoolboy error in creating this.

`Imports Microsoft.VisualBasic
Imports System.Data.SqlClient
Imports System.Collections.Generic

Public Class DB

Private _connectionString As String

Public Sub New(connectionString As String)
_connectionString = connectionString
End Sub

Public Sub ExecuteNonQuery(cmdTxt As String, params As Dictionary(Of String, Object))
Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
cmd.ExecuteNonQuery()
End Using
End Sub

Public Function ExecuteReader(cmdTxt As String, params As Dictionary(Of String, Object)) As SqlDataReader
Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
Return cmd.ExecuteReader()
End Using
End Function

Public Function ExecuteScalar(cmdTxt As String, params As Dictionary(Of String, Object)) As Object
Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
Return cmd.ExecuteScalar()
End Using
End Function

Private Function BuildCommand(cmdTxt As String, params As Dictionary(Of String, Object)) As SqlCommand
Using con As New SqlConnection(_connectionString)
Using cmd As SqlCommand = con.CreateCommand()
cmd.CommandType = CommandType.StoredProcedure
cmd.CommandText = cmdTxt

Now that's some coincidence! No later than this week I wrote an ADODB "wrapper" for some vb6 legacy code (here). This looks very much similar, only for ADO.NET.

I don't like the name. DB is not respecting the PascalCase naming convention for VB.NET classes (same in C#) - I realize it's a short handy name, but it looks like a variable's name. I'd rename the class to, say, AdoCommandBuilder; the name of the object should say what the object's role is. In this case, building an ADO.NET command.

I might be completely wrong here, but it seems to me that a thread entering BuildCommand from ExecuteReader would open up a connection, and then when the function returns, the connection would be disposed before the client code can start iterating the reader. Thus I wouldn't wrap an IDisposable in a Using block if I'm going to be returning it.

Anyway your command is being disposed twice. Once in BuildCommand:

Using cmd As SqlCommand = con.CreateCommand()
            cmd.CommandType = CommandType.StoredProcedure
            cmd.CommandText = cmdTxt
            AddParameters(cmd, params)
            con.Open()
            Return cmd
        End Using '<<<<<< here

...and once more in the ExecuteXxxxx methods:

Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
        Return cmd.ExecuteReader()
    End Using ' <<<<<< here too!

I'd remove the Using cmd As SqlCommand from the CreateCommand method.

I agree with @Malachi, that BuildCommand shouldn't be opening the connection - its concern is building a command, so it should be taking a connection as a parameter; by doing that you enable extending your class with methods that take a connection, which enables running your commands in a transaction.

Hence, I think I would drop the parameterized constructor, add a Connect(connectionString As String) method instead, encapsulate the connection (, implement IDisposable) and give each ExecuteXxxxxx method an overload that takes an IDbConnection parameter; if you're not going to be taking in a connection, then you should own the whole process and return the data, not the reader itself - the idea is that you generally want the object that creates an IDisposable to be the one calling Dispose on it, so I'd only return a SqlDataReader when the client code owns the connection.

StackExchange Code Review Q#46856, answer score: 8