Data Access Layer

Question

This code review request is regarding data access layer coding. I have used the method I am about to describe in a few smaller applications, but I am noticing limitations that I would like to avoid in larger applications I am working on.

The specific area of data access I am referring to is reading data and converting it into strongly typed objects for use in the application.

My method is as follows:

• First, define the object as a class (or Structure if only reading is required) with all of the required properties and methods that I need.
• Second, I will create a module with static functions that can be used to retrieve records from the database and return them as instances of the object they are. Examples of the static functions are listed in the code below.

The problem is that I am duplicating the code for each method that retrieves the same object just with different filters. In a small app this is not a problem because it is just a couple places, but in larger apps the code duplication is a bit of a pain. In other situations, I have refactored the code that converts the DataReader, but I feel like there must be a better way. Any ideas?

Strongly Typed Object - This example is short, but in practice I often have larger classes in a logic layer for these objects that include methods for saving, deleting, etc.

Public Structure ProdordOrder
 Dim OrderNumber As String
 Dim Batch As String
 Dim Material As String
 Dim Plant As String
End Structure

Static Functions for data retrieval - Notice they are the same, except for the filters. This is where I am having issues with code duplication that I would like to address.

Public Function getProdordOrder(OrderNumber As String,
 Optional ByVal DBConnection As String = "MQ_Cache1") As ProdordOrder
 Dim ProductionOrderRecord As New ProdordOrder
 Dim conn As New OracleConnection(ConnectionStrings(MESIConnection).ConnectionString)
 Dim cmd As New OracleCommand("SELECT * FROM X WHERE C0 = :OrderNumber", conn)
 Try
 cmd.CommandType = CommandType.Text
 cmd.Parameters.Add("OrderNumber", OrderNumber)
 cmd.BindByName = True
 conn.Open()
 Using dr As OracleDataReader = cmd.ExecuteReader
 If dr.Read() Then
 ProductionOrderRecord.OrderNumber = dr("C0").ToString()
 ProductionOrderRecord.Material = dr("C1").ToString()
 ProductionOrderRecord.Batch = dr("C2").ToString()
 ProductionOrderRecord.Plant = dr("C3").ToString()
 Else
 Throw New Exception("Production Order not found")
 End If
 End Using
 Catch ex As Exception
 Throw
 Finally
 If conn IsNot Nothing Then
 If conn.State = ConnectionState.Open Then
 conn.Close()
 End If
 End If
 End Try
 Return ProductionOrderRecord
End Function
Public Function getProductionOrder(MaterialNumber As String,
 BatchNumber As String,
 Optional ByVal DBConnection As String = "MQ_Cache1") As ProdordOrder
 Dim ProductionOrderRecord As New ProdordOrder
 Dim conn As New OracleConnection(ConnectionStrings(DBConnection).ConnectionString)
 Dim cmd As New OracleCommand("SELECT * FROM X WHERE C1 = :MaterialNumber AND C2 = :BatchNumber", conn)
 Try
 cmd.CommandType = CommandType.Text
 cmd.Parameters.Add("MaterialNumber", MaterialNumber)
 cmd.Parameters.Add("BatchNumber", BatchNumber)
 cmd.BindByName = True
 conn.Open()
 Using dr As OracleDataReader = cmd.ExecuteReader
 If dr.Read() Then
 ProductionOrderRecord.OrderNumber = dr("C0").ToString()
 ProductionOrderRecord.Material = dr("C1").ToString()
 ProductionOrderRecord.Batch = dr("C2").ToString()
 ProductionOrderRecord.Plant = dr("C3").ToString()
 Else
 Throw New Exception("Production Order not found")
 End If
 End Using
 Catch ex As Exception
 Throw
 Finally
 If conn IsNot Nothing Then
 If conn.State = ConnectionState.Open Then
 conn.Close()
 End If
 End If
 End Try
 Return ProductionOrderRecord
End Function

• reggaeguitar: Entity Framework might help

Answer 1

Drying

By extracting a method out of such similiar methods you can reduce the code duplication.

Right now these two methods differ only by the passed in method arguments and the querystring condition. If we extract the common parts out of the methods we are going in the right direction.

Let us see how such a extracted method could look like. At first we will change the naming of the method to use PascalCase casing because this is defined in the .NET naming guidelines, and according the same guidelines we will name the method parameters using camelCase casing.

This extracted method needs a collection of names and values representing the parameters which can be expressed at the best as a Dictionary(Of String, Object) and the query condition. The optional DBConnection which isn't used can be eleminated.

Public Function GetFilteredProdordOrder(parameters As Dictionary(Of String, Object), queryCondition As String) As ProdordOrder
End Function 

Next we extract the creation of the OracleCommand object to a separate method like so

Private Function GetOracleCommand(query As String, parameters As Dictionary(Of String, Object), connection As OracleConnection) As OracleCommand
 Dim cmd As OracleCommand = New OracleCommand(query, connection)
 cmd.CommandType = CommandType.Text
 For Each kvp As KeyValuePair(Of String, Object) In parameters
 cmd.Parameters.Add(kvp.Key, kvp.Value)
 Next
 cmd.BindByName = True
 Return cmd
End Function

which can be used by every method to get a OracleCommand by passing the said dictionary together with a querystring and a OracleConnection.

Taking into account the answer of @Dan Lyons the GetFilteredProdordOrder() method will look like so

Private Function GetFilteredProdordOrder(parameters As Dictionary(Of String, Object), queryCondition As String) As ProdordOrder
 Dim query As String = "SELECT * FROM X " & queryCondition
 Using conn As New OracleConnection(ConnectionStrings(MESIConnection).ConnectionString)
 Using cmd As OracleCommand = GetOracleCommand(query, parameters, conn)
 conn.Open()
 Using dr As OracleDataReader = cmd.ExecuteReader
 If Not dr.Read() Then
 Throw New Exception("Production Order not found")
 End if
 Dim productionOrderRecord As New ProdordOrder
 productionOrderRecord.OrderNumber = dr("C0").ToString()
 productionOrderRecord.Material = dr("C1").ToString()
 productionOrderRecord.Batch = dr("C2").ToString()
 productionOrderRecord.Plant = dr("C3").ToString()
 Return productionOrderRecord
 End Using
 End Using
 End Using
 Throw New Exception("Will never happen.")
End Function

Because one should declare a variable as near as possible to its usage, and returning early is a valid way out of the method, I have added the Throw New Exception("Will never happen.") at the end to overcome the waring in the IDE.

Now the former getProdordOrder() methods will look like so

Public Function GetProdordOrder(orderNumber As String) as ProdordOrder 
 Dim parameters as new Dictionary(Of String, Object)
 parameters.Add("OrderNumber", orderNumber)
 Dim queryCondition As String = "WHERE C0 = :OrderNumber"
 Return GetFilteredProdordOrder(parameters, queryCondition)
End Function
Public Function GetProdordOrder(materialNumber As String, batchNumber As String) as ProdordOrder 
 Dim parameters as new Dictionary(Of String, Object)
 parameters.Add("MaterialNumber", materialNumber)
 parameters.Add("BatchNumber", batchNumber)
 Dim queryCondition As String = "WHERE C1 = :MaterialNumber AND C2 = :BatchNumber"
 Return GetFilteredProdordOrder(parameters, queryCondition)
End Function

---

Some more comments on the code

The enclosing of code with a Try..Catch to catch the general Exception only to rethrow this Exception is superflous and only adds noise to the code. What is the sense of rethrowing here ? I don't see any. It not only doesn't serve any purpose, but increases the horizontal spacing which makes it much harder to read the code.

Answer 2

Object Disposal

The most glaring thing I notice is that you aren't disposing of everything you need to. There's a Using statement for your reader, but both IDbConnection and IDbCommand (implemented by OracleConnection and OracleCommand, respectively) extend IDisposable.

The upside is that with connection disposal, you don't have to worry about explicitly closing it, so the try/catch wrapping your code in both functions can go away.

Another fun catch: if you are using Oracle's ODP.NET or their managed provider library, OracleParameter is IDisposable, too.

End result, only addressing disposal (full disclosure - I rarely touch VB):

Public Function getProdordOrder(OrderNumber As String,
 Optional ByVal DBConnection As String = "MQ_Cache1") As ProdordOrder
 Dim ProductionOrderRecord As New ProdordOrder
 Using conn As New OracleConnection(ConnectionStrings(MESIConnection).ConnectionString)
 Using cmd As New OracleCommand("SELECT * FROM X WHERE C0 = :OrderNumber", conn)
 cmd.CommandType = CommandType.Text
 cmd.Parameters.Add("OrderNumber", OrderNumber)
 cmd.BindByName = True
 conn.Open()
 Using dr As OracleDataReader = cmd.ExecuteReader
 If dr.Read() Then
 ProductionOrderRecord.OrderNumber = dr("C0").ToString()
 ProductionOrderRecord.Material = dr("C1").ToString()
 ProductionOrderRecord.Batch = dr("C2").ToString()
 ProductionOrderRecord.Plant = dr("C3").ToString()
 Else
 Throw New Exception("Production Order not found")
 End If
 End Using
 End Using
 End Using
 Return ProductionOrderRecord
End Function
Public Function getProductionOrder(MaterialNumber As String,
 BatchNumber As String,
 Optional ByVal DBConnection As String = "MQ_Cache1") As ProdordOrder
 Dim ProductionOrderRecord As New ProdordOrder
 Using conn As New OracleConnection(ConnectionStrings(DBConnection).ConnectionString)
 Using cmd As New OracleCommand("SELECT * FROM X WHERE C1 = :MaterialNumber AND C2 = :BatchNumber", conn)
 cmd.CommandType = CommandType.Text
 Using materialParameter = cmd.Parameters.Add("MaterialNumber", MaterialNumber)
 Using batchParameter = cmd.Parameters.Add("BatchNumber", BatchNumber)
 cmd.BindByName = True
 conn.Open()
 Using dr As OracleDataReader = cmd.ExecuteReader
 If dr.Read() Then
 ProductionOrderRecord.OrderNumber = dr("C0").ToString()
 ProductionOrderRecord.Material = dr("C1").ToString()
 ProductionOrderRecord.Batch = dr("C2").ToString()
 ProductionOrderRecord.Plant = dr("C3").ToString()
 Else
 Throw New Exception("Production Order not found")
 End If
 End Using
 End Using
 End Using
 End Using
 End Using
 Return ProductionOrderRecord
End Function

I don't know your feelings on indenting using statements which are right next to one another, so I left it with default formatting.

Answer 3

For the DR section, you could have a function that returns your object. The same function could then be used for any query that returns a list of that same class.

 Using dr As OracleDataReader = cmd.ExecuteReader
 If dr.Read() Then
 ProductionOrderRecord = GetNewProductionOrderRecord(dr)
 Else
 Throw New Exception("Production Order not found")
 End If
 End Using
Private Function ProductionOrderRecord(ByVal dr As OracleDataReader) As ProdordOrder
 Dim newOrder As New ProdordOrder
 ProductionOrderRecord.OrderNumber = dr("C0").ToString()
 ProductionOrderRecord.Material = dr("C1").ToString()
 ProductionOrderRecord.Batch = dr("C2").ToString()
 ProductionOrderRecord.Plant = dr("C3").ToString()
 Return newOrder
End Function

Micro-optimization: For query that returns list, you could get the ordinal value instead of using the actual string of the column.