Database access class using VB.NET

Question

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
                AddParameters(cmd, params)
                con.Open() 'working on the assumption this command will be run as soon as it's retuned; so this open is left as late as possible but here to avoid duplicate code
                Return cmd
            End Using
        End Using
    End Function

    Private Sub AddParameters(ByRef cmd As SqlCommand, params As Dictionary(Of String, Object))
        If Not params Is Nothing Then
            For Each kvp As KeyValuePair(Of String, Object) In params
                cmd.Parameters.AddWithValue(kvp.Key, kvp.Value)
            Next
        End If
    End Sub

End Class

Answer 1

This looks really good and clean to me as well, but I too come from a C# Background.

I think there are only a couple of things missing that I would personally like to see myself.

1. Error Handling
2. Constructor

there is only one thing that I would probably change.

your BuildCommand Function should not open the connection. I think that the open and close should be done in the Execute functions.

Answer 2

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.