Code Review: ADO.NET Data Access Utility Class (VB)

Question

When I started at my current employer I inherited a project from a previous developer and in that project was a data access utility class that tries to simplify a lot of the code involved in making calls and retrieving data back from the database. Over time it has been modified to add more overloaded forms of functions, and now I am looking at possible suggestions from the StackOverflow community.

What do you think should be added? Removed? Modified?

Note:

It would be nice if this class could remain compatible to VB.NET in the .NET 2.0 framework. We are also developing in 3.5, but I would like to have something that is generally going to work across most frameworks (so no LINQ, etc.) Also, please refrain from unnecessary answers that consist of nothing but "Use nHibernate" or other tools.

My class:

Public Class DataAccess
    Public Shared Function ReturnScalar(ByVal CmdStr As String) As String
        Dim Result As String
        Dim con As SqlConnection = New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As SqlCommand = New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        Result = cmd.ExecuteScalar
        con.Close()
        Return Result
    End Function

    Public Shared Function ReturnScalar(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As Integer) As String
        Dim Result As String
        Dim con As SqlConnection = New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As SqlCommand = New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        Result = cmd.ExecuteScalar
        con.Close()
        Return Result
    End Function

    Public Shared Function ReturnScalar(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As String) As String
        Dim Result As String
        Dim con As SqlConnection = New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As SqlCommand = New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        Result = cmd.ExecuteScalar
        con.Close()
        Return Result
    End Function

    Public Shared Function ReturnScalar(ByVal CmdStr As String, ByVal Params As Hashtable) As String
        Dim Result As String
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        For Each entry As DictionaryEntry In Params
            cmd.Parameters.Add(New SqlParameter(entry.Key.ToString, entry.Value))
        Next
        Result = cmd.ExecuteScalar
        con.Close()
        Return Result
    End Function

    Public Shared Function ReturnDataSet(ByVal CmdStr As String) As DataSet
        Dim DS As New DataSet
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        adp.Fill(DS)
        con.Close()
        Return DS
    End Function

    Public Shared Function ReturnDataSet(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As Integer) As DataSet
        Dim DS As New DataSet
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)

        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        adp.Fill(DS)
        con.Close()
        Return DS
    End Function

    Public Shared Function ReturnDataSet(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As String) As DataSet
        Dim DS As New DataSet
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)    
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        adp.Fill(DS)
        con.Close()
        Return DS
    End Function

    Public Shared Function ReturnDataSet(ByVal CmdStr As String, ByVal Params As Hashtable) As DataSet
        Dim DS As New DataSet
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        For Each entry As DictionaryEntry In Params
            cmd.Parameters.Add(New SqlParameter(entry.Key.ToString, entry.Value))
        Next
        adp.Fill(DS)
        con.Close()
        Return DS
    End Function

    Public Shared Function ReturnDataTable(ByVal CmdStr As String) As DataTable
        Dim DT As New DataTable
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        adp.Fill(DT)
        con.Close()
        Return DT
    End Function

    Public Shared Function ReturnDataTable(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As Integer) As DataTable
        Dim DT As New DataTable
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        adp.Fill(DT)
        con.Close()
        Return DT
    End Function

    Public Shared Function ReturnDataTable(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As Date) As DataTable
        Dim DT As New DataTable
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        adp.Fill(DT)
        con.Close()
        Return DT
    End Function

    Public Shared Function ReturnDataTable(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As String) As DataTable
        Dim DT As New DataTable
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        adp.Fill(DT)
        con.Close()
        Return DT
    End Function

    Public Shared Function ReturnDataTable(ByVal CmdStr As String, ByVal Params As Hashtable) As DataTable
        Dim DT As New DataTable
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        Dim adp As New SqlDataAdapter(cmd)
        cmd.CommandType = CommandType.StoredProcedure
        For Each entry As DictionaryEntry In Params
            cmd.Parameters.Add(New SqlParameter(entry.Key.ToString, entry.Value))
        Next
        adp.Fill(DT)
        con.Close()
        Return DT
    End Function

    Public Shared Function DBExecute(ByVal CmdStr As String) As Integer
        Dim RowsAffected As Integer
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        RowsAffected = cmd.ExecuteNonQuery
        con.Close()
        Return RowsAffected
    End Function

    Public Shared Function DBExecute(ByVal CmdStr As String, ByVal ParamName As String, ByVal Param As Integer) As Integer
        Dim RowsAffected As Integer
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.Add(New SqlParameter(ParamName, Param))
        RowsAffected = cmd.ExecuteNonQuery
        con.Close()
        Return RowsAffected
    End Function

    Public Shared Function DBExecute(ByVal CmdStr As String, ByVal Params As Hashtable) As Integer
        Dim RowsAffected As Integer
        Dim con As New SqlConnection(ConfigurationManager.ConnectionStrings("RoutingConnectionString").ToString)
        con.Open()
        Dim cmd As New SqlCommand(CmdStr, con)
        cmd.CommandType = CommandType.StoredProcedure
        For Each entry As DictionaryEntry In Params
            cmd.Parameters.Add(New SqlParameter(entry.Key.ToString, entry.Value))
        Next
        RowsAffected = cmd.ExecuteNonQuery
        con.Close()
        Return RowsAffected
    End Function
End Class

Note #2:

"RoutingConnectionString" just happens to be the name of the ConnectionString in this particular version of the class and is not something which would remain the same throughout all of our projects.

Note #3:

All of our projects utilize SQLServer2005, but I do not see anything wrong with modifications which could help make it more flexible with other database systems such as MySQL or older/newer versions of SQL Server.

Answer 1

It's going to take a little while to study the class above. However, a very similar question was asked about a month ago on my discussion group and you may find it worthwhile to read the answers.

In the meantime, I'll load up your class and analyse it. When done, I'll edit my post to include my comments.

---

I'm hoping you've taken a look at the thread I linked to so I will not be listing points mentioned there that are relevant here too. No point repeating myself when this is going to be a long post anyway! ;-)

Problem 1: Construction/Destruction : The class appears to have no public constructor and therefore the initialization logic is repeated within each called method. This makes this class more of a Static Class. I do not believe that this is the best design decision for a DataLayer class, which is usually intended to be called simultaneously from hundreds of classes/modules/libraries/pages.

Secondly, it has no disposition logic and no finalizer. The dispose pattern is so well agreed upon that VS actually inserts the template into your code the moment you implement IDisposable. Database related classes should provide for dispose.

Problem 2: No Exception handling:

2a) Try-Catch: There is not a single Try-Catch-Finally block in the class. Your class will not be able to gracefully handle exceptions and pass on useful information to the calling code. If an exception occurs during database access, your connection will remain open. Using - End Using constructs are very much recommended and they are basically an abstraction of the Try-Finally pattern.

2b) Error logging: Your code should have some sort of logging mechanism though this might also be implemented by the calling code if exceptions are thrown from this class.

Problem 3: Excessive/Unneeded Overloads: I don't really see the need to have the presented overloads.

3a) Repetition: Much of the code in the overloads seems to be repetitive. I think it could be handled in a single method with checks for nulls. Or it could be extracted into private methods.

For instance:

Public Shared Function DBExecute(ByVal CmdStr As String, ByVal Params As Hashtable) As Integer
  ...basic initialization
  If Params isnot Nothing then
    For Each entry As DictionaryEntry In Params
      cmd.Parameters.Add(New SqlParameter(entry.Key.ToString, entry.Value))
    Next
  End If
  '...rest of processing.
End Function

3b) Dispensability: Secondly, many of your overloads simply result from the changed datatype of the Parameter. To resolve this problem, you may find it easier to reflect upon the base type of the value of the item in the HashTable. Another way that I have used in the past is to have a single method signature and allow the user to pass in an array of SqlParameters themselves. This allows the user the complete freedom to set properties such as datatypes, size, direction, nullability, and precision. A third method that I use currently is to have a settable property of type SqlParameterCollection that would be set by the end user and evaluated before executing each kind of database method.

Finally, I see no point in having separate methods (and overloads) for ReturnDataSet and ReturnDataTable, since the former will return a collection of DataTables in any case.

Problem 4: Connection reuse: The use of a Shared class pattern without a constructor means that a user cannot run consequent queries using the same object without having to go through the same initialization.

Problem 5: Stored Procedure support: The CommandType of an SqlCommand has no way to be set to Text. All Commands are assumed to be of type StoredProcedure.

Problem 6: ConnectionStrings:

6a) Encryption: The class provides no support for encrypted connection strings in the configuration file.

6b) Configurability: You mentioned that the name of the Connectionstring key can differ. How will that be set without having to recompile the class?

Problem 7: DataAdapter connections: When using DataAdapters (such as in the ReturnDataSet methods), it's always better to let the DataAdapter manage the opening and closing of the Connection.

Problem 8: ReturnScalar datatypes: Your ReturnScalar methods assume a return type of String. Firstly, the explicit cast is missing (My "Option Explicit On" global setting immediately flagged this error). Secondly, what of other datatypes ? I think you should return an Object and let the calling code cast as required.

Problem 9: Xml support: None of the methods allow invocation of the ExecuteXmlReader method which is specific to the SqlClient provider. You may not need this in your scenario, but I thought it's worth a mention.

I guess that about wraps it up. Please don't mind this, but if you were to ask me to rate this class, I would give it a 3 on 10. Those 3 are for good intentions and for the commendable initiative to encapsulate database access logic into a separate library.

Answer 2

A few observations:

1) You can make your execution routines more flexible by using a ParamArray as an argument. That way you can have just a handful of methods that can handle different types of SQL parms and variable numbers of them. The caller can simply pass in however many values a given stored procedure needs, then you can use SqlCommandBuilder.DeriveParameters to populate your SqlCommand object. The caller would have to make sure they passed the parms in the order the SP expects them, but that should be acceptable.

2) It looks like all SqlCommand executions use the default timeout. There may be cases where you need to increase it, so having that as an optional argument to ReturnDataSet/ReturnDataTable/etc. might be advantageous.

3) You might consider having a separate set of routines that do the same kinds of things, but inside a transaction.