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.

• Mr. C: This rocks! I'm extremely new to the whole automated testing idea (I just picked it up two weeks ago) and your post was very insightful. I'll make the suggested changes and check out your introduction. Thanks a million!
• RubberDuck: @Mr.C may I recommend my favorite mocking library, Moq? There's a little bit of a learning curve if you're not familiar with the lambda syntax, but it makes it very easy to fake external dependencies.

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.

• Malachi: I am not following you here. if you create a method that returns the Datatable to the caller you should still make sure that when the method is finished with the DataSet or DataTable that it disposes what is there. the caller should be using its own Using block to receive the DataTable or DataSet from the method it calls, so it is actually 2 separate items, one in the Method and one in the Caller.
• Mathieu Guindon: @Malachi wrong. Calling into a disposed object is a good way to blow things up - you can't dispose a disposable that you're passing around :)
• Malachi: I see my error in thought. the object is created in the method and shouldn't be destroyed when the method is completed.