7
\$\begingroup\$

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
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Entity Framework might help \$\endgroup\$ Aug 11, 2015 at 17:03

3 Answers 3

3
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.