Data Access Layer code for MSSQL Databases

Question

We use this class quite a bit for boilerplate / glue that allows web services and applications to call databases. This code is very important, and I'd like to ensure I'm getting the best performance possible.

Any and all feedback is welcome! If there is a better / more efficient way of doing something, please let me know.

Imports System.Data.SqlClient
Imports System.Data

Public Class MSSqlUtility

#Region "Data Access Utilities"

    ''' <summary>
    ''' Gets the data table.
    ''' </summary>
    ''' <param name="_command">The _command.</param>
    ''' <param name="_commandType">Type of the _command.</param>
    ''' <param name="_prmList">The parameter list.</param>
    ''' <returns>DataTable object - returns empty datatable if it failed</returns>
    Public Shared Function GetDataTable(ByVal _command As String, ByVal _commandType As CommandType, Optional ByVal _prmList As ArrayList = Nothing) As DataTable

        Dim cmd As SqlCommand = Nothing
        Dim conn As SqlConnection = Nothing
        Try
            Using dt As New DataTable()
                conn = New SqlConnection(GetConnectionString())
                cmd = New SqlCommand(_command, conn)
                cmd.CommandType = _commandType

                If (_prmList IsNot Nothing) Then
                    For Each p As SqlParameter In _prmList
                        cmd.Parameters.Add(p)
                    Next
                End If

                If (conn.State <> ConnectionState.Open) Then
                    conn.Open()
                End If
                'Fill the datatable object
                Using adapter As New SqlDataAdapter(cmd)
                    adapter.Fill(dt)
                End Using
                If (conn.State <> ConnectionState.Closed) Then
                    conn.Close()
                End If
                Return dt
            End Using
        Catch ex As Exception
            LoggingUtility.LogError(ex, "MSSqlUtility.GetDataTable")
            Return New DataTable()
        Finally
            cmd.Connection.Close()
            cmd.Parameters.Clear()
            cmd.Dispose()
            conn.Dispose()
        End Try

    End Function

    ''' <summary>
    ''' Gets the data set.
    ''' </summary>
    ''' <param name="_command">The _command.</param>
    ''' <param name="_commandType">Type of the _command.</param>
    ''' <param name="_prmList">The _PRM list.</param>
    ''' <returns>DataSet object - empty if an error occurred</returns>
    Public Shared Function GetDataSet(ByVal _command As String, ByVal _commandType As CommandType, Optional ByVal _prmList As ArrayList = Nothing) As DataSet

        Dim cmd As SqlCommand = Nothing
        Dim conn As SqlConnection = Nothing
        Try
            Using dt As New DataSet()
                conn = New SqlConnection(GetConnectionString())
                cmd = New SqlCommand(_command, conn)
                cmd.CommandType = _commandType

                If (_prmList IsNot Nothing) Then
                    For Each p As SqlParameter In _prmList
                        cmd.Parameters.Add(p)
                    Next
                End If

                If (conn.State <> ConnectionState.Open) Then
                    conn.Open()
                End If
                'Fill the DataSet object
                Using adapter As New SqlDataAdapter(cmd)
                    adapter.Fill(dt)
                End Using
                If (conn.State <> ConnectionState.Closed) Then
                    conn.Close()
                End If
                Return dt
            End Using
        Catch ex As Exception
            LoggingUtility.LogError(ex, "MSSqlUtility.GetDataSet")
            Return New DataSet()
        Finally
            cmd.Connection.Close()
            cmd.Parameters.Clear()
            cmd.Dispose()
            conn.Dispose()
        End Try

    End Function

    ''' <summary>
    ''' Gets the very first item returned by a query or command.
    ''' </summary>
    ''' <param name="_command">The _command.</param>
    ''' <param name="_commandType">Type of the _command.</param>
    ''' <param name="_prmList">The _PRM list.</param>
    ''' <returns>object - you will need to cast this appropriately. Returns -1 if an error occured.</returns>
    Public Shared Function GetScalar(ByVal _command As String, ByVal _commandType As CommandType, Optional ByVal _prmList As ArrayList = Nothing) As Object

        Dim cmd As SqlCommand = Nothing
        Dim conn As SqlConnection = Nothing
        Dim obj As New Object()
        Try
            conn = New SqlConnection(GetConnectionString())
            cmd = New SqlCommand(_command, conn)
            cmd.CommandType = _commandType

            If (_prmList IsNot Nothing) Then
                For Each p As SqlParameter In _prmList
                    cmd.Parameters.Add(p)
                Next
            End If
            If (conn.State <> ConnectionState.Open) Then
                conn.Open()
            End If
            obj = cmd.ExecuteScalar()
            If (conn.State <> ConnectionState.Closed) Then
                conn.Close()
            End If

        Catch ex As Exception
            LoggingUtility.LogError(ex, "MSSqlUtility.GetScalar")
            'return -1 to identify an error
            Return -1
        Finally
            cmd.Connection.Close()
            cmd.Parameters.Clear()
            cmd.Dispose()
            conn.Dispose()
        End Try
        Return obj
    End Function

    ''' <summary>
    ''' Executes the SQL item.
    ''' </summary>
    ''' <param name="_command">The _command.</param>
    ''' <param name="_commandType">Type of the _command.</param>
    ''' <param name="_prmList">The _PRM list. This should be of SqlParameter type.</param>
    Public Shared Sub ExecuteSqlItem(ByVal _command As String, ByVal _commandType As CommandType, Optional ByVal _prmList As ArrayList = Nothing)

        Dim cmd As SqlCommand = Nothing
        Dim conn As SqlConnection = Nothing
        Try
            conn = New SqlConnection(GetConnectionString())
            cmd = New SqlCommand(_command, conn)
            cmd.CommandType = _commandType

            If (_prmList IsNot Nothing) Then
                For Each p As SqlParameter In _prmList
                    cmd.Parameters.Add(p)
                Next
            End If

            If (conn.State <> ConnectionState.Open) Then
                conn.Open()
            End If
            cmd.ExecuteNonQuery()
            If (conn.State <> ConnectionState.Closed) Then
                conn.Close()
            End If
        Catch ex As Exception
            LoggingUtility.LogError(ex, "MSSqlUtility.ExecuteSqlItem")
        Finally
            cmd.Connection.Close()
            cmd.Parameters.Clear()
            cmd.Dispose()
            conn.Dispose()
        End Try

    End Sub

#End Region

End Class

To test the class, we use this automated test class. I have cleared the stored procedure and parameter names - you will need to adjust these to test the code above. This code is not what we are concerned with, but if you're adamant on code reviews, I suppose you can review this too :)

    [TestMethod]
    public void GetDataTableTest()
    {
        var procName = "your_stored_procedure_name";
        var dt = MSSqlUtility.GetDataTable(procName, CommandType.StoredProcedure);

        Assert.IsFalse(dt == null);
        Assert.IsTrue(dt.Rows.Count > 0);

        dt = null;
        procName = "your_stored_procedure_name";

        var prmList = new ArrayList();
        prmList.Add(new SqlParameter("@parameter", value));

        dt = MSSqlUtility.GetDataTable(procName, CommandType.StoredProcedure, prmList);

        Assert.IsFalse(dt == null);
        Assert.IsTrue(dt.Rows.Count > 0);
    }
    [TestMethod]
    public void GetDataSetTest()
    {
        var procName = "your_stored_procedure_name";
        DataSet ds = null;

        ds = MSSqlUtility.GetDataSet(procName, CommandType.StoredProcedure);

        Assert.IsFalse(ds == null);
        Assert.IsFalse(ds.Tables[0] == null);
        Assert.IsTrue(ds.Tables[0].Rows.Count > 0);
    }
    [TestMethod]
    public void GetScalarTest()
    {
        var procName = "your_stored_procedure_name";
        var prmList = new ArrayList();
        prmList.Add(new SqlParameter("@parameter", value));
        object result = null;

        result = MSSqlUtility.GetScalar(procName, CommandType.StoredProcedure, prmList);

        Assert.IsFalse(result == null);
        int i = 0;
        Int32.TryParse(result.ToString(), out i);

        Assert.IsTrue(i > 0);

    }
    [TestMethod]
    public void ExecuteSqlItemTest()
    {
        var procName = "your_stored_procedure_name";
        var inputs = new ArrayList();
        inputs.Add(new SqlParameter("@parameter", value));
        inputs.Add(new SqlParameter("@parameter", value));
        inputs.Add(new SqlParameter("@parameter", value));
        inputs.Add(new SqlParameter("@parameter", value));

        try
        {
            MSSqlUtility.ExecuteSqlItem(procName, CommandType.StoredProcedure, inputs);
            //Pass the test. Assume the data manipulation was successful. 
            //Recommend verify the data changes in the database in addition to running this test.
            Assert.IsTrue(true);
        }
        catch
        {
            //Something failed and went wrong. Fail the test.
            Assert.IsTrue(false);
        }
    }

Answer 1

This whole function needs a little bit of work:

    Dim cmd As SqlCommand = Nothing
    Dim conn As SqlConnection = Nothing
    Try
        Using dt As New DataTable()
            conn = New SqlConnection(GetConnectionString())
            cmd = New SqlCommand(_command, conn)
            cmd.CommandType = _commandType

            If (_prmList IsNot Nothing) Then
                For Each p As SqlParameter In _prmList
                    cmd.Parameters.Add(p)
                Next
            End If

            If (conn.State <> ConnectionState.Open) Then
                conn.Open()
            End If
            Using adapter As New SqlDataAdapter(cmd)
                adapter.Fill(dt)
            End Using
            If (conn.State <> ConnectionState.Closed) Then
                conn.Close()
            End If
            Return dt
        End Using
    Catch ex As Exception
        LoggingUtility.LogError(ex, "MSSqlUtility.GetDataTable")
        Return New DataTable()
    Finally
        cmd.Connection.Close()
        cmd.Parameters.Clear()
        cmd.Dispose()
        conn.Dispose()
    End Try

you should add some using statements here, like a lot of them.

First thing is that we use more using statements and get rid of the finally statement, like this:

    Dim cmd As SqlCommand = Nothing
    Dim conn As SqlConnection = Nothing
    Try
        Dim dt As New DataTable()
        Using conn As New SqlConnection(GetConnectionString())
            Using cmd = New SqlCommand(_command, conn)
                cmd.CommandType = _commandType

                If (_prmList IsNot Nothing) Then
                    For Each p As SqlParameter In _prmList
                        cmd.Parameters.Add(p)
                    Next
                End If

                If (conn.State <> ConnectionState.Open) Then
                    conn.Open()
                End If
                Using adapter As New SqlDataAdapter(cmd)
                    adapter.Fill(dt)
                End Using
                If (conn.State <> ConnectionState.Closed) Then
                    conn.Close()
                End If
                Return dt
            End Using
        End Using
    Catch ex As Exception
        LoggingUtility.LogError(ex, "MSSqlUtility.GetDataTable")
        Return New DataTable()
    End Try

Then we are going to get rid of:

 If (conn.State <> ConnectionState.Open) Then
     conn.Open()
 End If

and this:

If (conn.State <> ConnectionState.Closed) Then
    conn.Close()
End If

They are not needed, at all. In fact, you don't even need to manually close anything declared with Using.

Now we have this:

Dim cmd As SqlCommand = Nothing
Dim conn As SqlConnection = Nothing
Try
    Dim dt As New DataTable()
    Using conn As New SqlConnection(GetConnectionString(
        Using cmd = New SqlCommand(_command, conn)
            cmd.CommandType = _commandType

            If (_prmList IsNot Nothing) Then
                For Each p As SqlParameter In _prmList
                    cmd.Parameters.Add(p)
                Next
            End If

            Using adapter As New SqlDataAdapter(cmd)
                adapter.Fill(dt)
            End Using
            Return dt
        End Using
    End Using
Catch ex As Exception
    LoggingUtility.LogError(ex, "MSSqlUtility.GetDataTable")
    Return New DataTable()
End Try

Wash, Rinse, and Repeat until all 3 look this clean, please.

I took another answers advice and removed the using block for the DataTable that is being returned. You may want to look into another way of returning the data in that DataTable though, as passing around something that is disposable like that can cause connection issues.

Answer 2

I will review the tests because they definitely need some work!

---

GetDataTableTest()

The Test suffix is redundant, they're all tests. What I want to know is what scenario is being tested when the test fails. For this reason I prefer to following the [UnitUnderTest]_[Scenario]_[ExpectedResult] scheme. An example could be GetDataTable_WithInvalidProcedure_ThrowsSqlException().

Normal method naming conventions do not apply in unit tests because these methods will never be manually called from code; their only purpose is to be displayed in the test runner.

---

Assert.IsFalse(dt == null);

Your code is filled with this and it's really not good: it hides all assert information and turns it into a true/false result. Why are you throwing information away? Even with a basic example as this test you'll already run into trouble: if an assert fails, you won't know what one.

Instead use Assert.IsNotNull(dt) and retain the information.

---

Assert.IsTrue(dt.Rows.Count > 0);

While we're on the topic: FluentAssertions. It turns that into something like dt.Rows.Should().NotBeEmpty(): it's easy to read and perhaps more importantly it also gives really useful information when the test fails, unlike "false".

---

var prmList = new ArrayList();

Is this intentional? You should never use the non-generic collection type unless you're interacting with an outdated spec. I can't tell if this is the case though, but at least you know it.

---

object result = null;
result = MSSqlUtility.GetScalar(procName, CommandType.StoredProcedure, prmList);

Assert.IsFalse(result == null);
int i = 0;
Int32.TryParse(result.ToString(), out i);

Assert.IsTrue(i > 0);

This is some messy code.

First of all: declare and initialize result on the same line unless there's a good reason not to.

Secondly: is there an option for you to return something other than object? No generic variant?

Thirdly: Don't do an intermediate Assert when it is not asserting the end-goal of the test. Just let it throw an exception because that means the test did its job well: it throws on unexpected results.

Lastly: Don't use TryParse. You expect it to parse so you want to force it to either parse or die. In your scenario, if result would have value 0 then the test would fail when it shouldn't because you use such a brittle way of checking if the parse succeeds.

---

try
{
    MSSqlUtility.ExecuteSqlItem(procName, CommandType.StoredProcedure, inputs);
    //Pass the test. Assume the data manipulation was successful. 
    //Recommend verify the data changes in the database in addition to running this test.
    Assert.IsTrue(true);
}
catch
{
    //Something failed and went wrong. Fail the test.
    Assert.IsTrue(false);
}

That is not how we do things around these parts. Several things very worrisome about this:

• No try-catches in a test. If an exception is thrown then something is by definition not going as expected. Try-catches and exceptions are meant for unexpected input or unexpected results. There's nothing unexpected about a scenario where you stub dependencies and have full control over what goes in.

• NO ASSUMPTIONS! This is in effect an entirely useless test because it doesn't test a thing. It just executes code.

• A good unit test should be repeatable. Are you going to check the database every time you execute your test suite? I hope not because that would mean that you either lose a lot of time or you barely execute your tests; both are unacceptable. And even then: your database should be cleaned after each test; how exactly are you going to check its contents at the end of a test run?

• Do not test against a database unless there is no other option whatsoever: it slows your tests down by a large difference. Furthermore the chance of tests interfering with eachother is heightened: do you clean up everything from the database to give the next test a clean slate?

---

I wrote an introduction to unit testing a few hours ago, give it a read because there is certainly room for improvement! I do like that they are rather small and that you separated the AAA stadia.

Answer 3

IDisposable

Using dt As New DataSet()
    ...
    return dt
End Using


Using dt As New DataTable()
    ...
    return dt
End Using

Using-blocks will call IDisposable.Dispose on the resource they are declared with. You should not dispose something you are going to return. It doesn't throw any ObjectDisposedException in this case but it could have.