0
\$\begingroup\$

Context: The following code is used to look up a setting values based on multiple layers of configuration within the application. For any given setting (ie: default printer), the application can be configured at the following levels in order of precedence.

  1. Operation
  2. Material
  3. Workstation & Program Combination
  4. Workstation
  5. Program
  6. User
  7. Plant

When the application requires a setting, it performs a lookup by providing as much information as it has available in context then searches each level and uses the first value it finds. If no value is found, the caller will handle the default action.

Logic Layer

This is the function that will be called by any part of the application that needs to look up a setting value. The caller must specify the setting ID and one or more of the other parameters. The purpose here is to allow the caller to look up just "Workstation" level settings for example.

 Public Function SettingLookup(operationID As String,
                              materialID As String,
                              userID As String,
                              plantID As String,
                              workstationID As String,
                              programID As String,
                              settingID As String
                              ) As String()

    If settingID = "" Then
        Throw New Exception("Setting ID Required")
    End If

    Dim settingValue = SettingLookupHelper(operationID, materialID, userID, plantID, workstationID, programID, settingID)

    If settingValue IsNot Nothing Then
        Return JsonConvert.DeserializeObject(Of String())(settingValue)
    Else
        Return Nothing
    End If

End Function

This function is used to access the data access layer and get the settings from the database.

Private Function SettingLookupHelper(operationID As String,
                                     materialID As String,
                                     userID As String,
                                     plantID As String,
                                     workstationID As String,
                                     programID As String,
                                     settingID As String
                                     ) As String

    Dim settingValue As Setting

    If operationID <> "" Then
        settingValue = GetOperationSetting(operationID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If materialID <> "" Then
        settingValue = GetMaterialSetting(materialID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If workstationID <> "" And programID <> "" Then
        settingValue = GetWorkstationProgramSetting(workstationID, programID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If workstationID <> "" Then
        settingValue = GetWorkstationSetting(workstationID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If programID <> "" Then
        settingValue = GetProgramSetting(programID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If userID <> "" Then
        settingValue = GetUserSetting(userID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    If plantID <> "" Then
        settingValue = GetPlantSetting(plantID, settingID)

        If settingValue IsNot Nothing Then
            Return settingValue.Value
        End If
    End If

    Return Nothing

End Function

Data Access Layer

This is an example of the code I have in the data access layer for each of the configuration levels listed above. Each of these files contains code to access a Settings table where the settings are defined and a many-to-many join table (ie: Material_Settings or Operation_Settings) that is used to set the setting values and link it to the entity like a material, operation, or user. There is a lot of nearly duplicated code in my data access layer to support this design.

Public Class MaterialSetting
    Inherits Setting

    Private _MaterialID As String
    Public Property Material_ID() As String
        Get
            Return _MaterialID
        End Get
        Set(ByVal value As String)
            _MaterialID = value
        End Set
    End Property


    Private _Material As String
    Public Property Material() As String
        Get
            Return _Material
        End Get
        Set(ByVal value As String)
            _Material = value
        End Set
    End Property


End Class


Public Module MaterialSettingsUtilities


    Public Function GetMaterialSetting(materialID As String, settingID As String) As MaterialSetting

        Dim parameters As New Dictionary(Of String, Object)
        parameters.Add("Material_ID", materialID)
        parameters.Add("Setting_ID", settingID)

        Dim queryCondition As String
        queryCondition = " WHERE Material_ID = HEXTORAW(:Material_ID) AND Setting_ID = HEXTORAW(:Setting_ID) "

        Dim settings As Settings = GetMaterialSettingsFiltered(parameters, queryCondition)

        If settings.Count > 0 Then
            Return CType(settings(0), MaterialSetting)
        Else
            Return Nothing
        End If

    End Function


    Private Function GetMaterialSettingsFiltered(parameters As Dictionary(Of String, Object), queryCondition As String) As Settings

        Dim query As String
        query = " SELECT * "
        query += "  FROM (Materials NATURAL JOIN Material_Settings) "
        query += "       NATURAL JOIN Settings "
        query += queryCondition

        Using conn As New OracleConnection(GetConnectionString("WeighScaleDB"))
            Using cmd = CreateOracleCommand(query, parameters, conn)

                conn.Open()

                Using dr = cmd.ExecuteReader

                    Dim settings As New Settings

                    If Not dr.Read() Then
                        Return settings
                    End If

                    Do
                        settings.Add(New MaterialSetting With {
                                     .Setting_ID = ConvertByteArrayToString(TryCast(dr("Setting_ID"), Byte())),
                                     .Material_ID = ConvertByteArrayToString(TryCast(dr("Material_ID"), Byte())),
                                     .Material = dr("Material").ToString(),
                                     .Value = dr("Setting_Value").ToString(),
                                     .Name = dr("Setting_Name").ToString(),
                                     .Description = dr("Setting_Description").ToString(),
                                     .DataType = dr("Setting_Data_Type").ToString(),
                                     .Category = dr("Setting_Category").ToString()
                                 })
                    Loop While dr.Read()

                    Return settings

                End Using
            End Using
        End Using

    End Function


End Module

This is the Settings object from which each of the subtypes (ie: MaterialSetting) are derived.

Public Class Setting

    Private _SettingID As String
    Public Property Setting_ID() As String
        Get
            Return _SettingID
        End Get
        Set(ByVal value As String)
            _SettingID = value
        End Set
    End Property

    Private _SettingName As String
    Public Property Name() As String
        Get
            Return _SettingName
        End Get
        Set(ByVal value As String)
            _SettingName = value
        End Set
    End Property

    Private _SettingDescription As String
    Public Property Description() As String
        Get
            Return _SettingDescription
        End Get
        Set(ByVal value As String)
            _SettingDescription = value
        End Set
    End Property

    Private _SettingDataType As String
    Public Property DataType() As String
        Get
            Return _SettingDataType
        End Get
        Set(ByVal value As String)
            _SettingDataType = value
        End Set
    End Property

    Private _SettingCategory As String
    Public Property Category() As String
        Get
            Return _SettingCategory
        End Get
        Set(ByVal value As String)
            _SettingCategory = value
        End Set
    End Property

    Private _SettingValue As String
    Public Property Value() As String
        Get
            Return _SettingValue
        End Get
        Set(ByVal value As String)
            _SettingValue = value
        End Set
    End Property        
End Class

Any constructive thoughts on would be appreciated. I can post examples of my table structures or any of the other helper or DAL functions shown in this code.

\$\endgroup\$
  • \$\begingroup\$ May I ask why you're not using Encoding.GetString? \$\endgroup\$ – Bjørn-Roger Kringsjå Nov 5 '15 at 18:13
  • \$\begingroup\$ @Bjørn-RogerKringsjå Thanks for the tip. I didn't know about that function. \$\endgroup\$ – Kent Anderson Nov 5 '15 at 19:13
3
\$\begingroup\$

SettingLookup

If settingID = "" Then
    Throw New Exception("Setting ID Required")
End If  
  • You shouldn't just throw an Exception but you should throw an ArgumentException. From the design guidelines for exception

    Do throw the most specific (the most derived) exception that is appropriate. For example, if a method receives a null (Nothing in Visual Basic) argument, it should throw System.ArgumentNullException instead of its base type System.ArgumentException.

  • IMO you should test if the settingID is null (Nothing) too.

     If settingID Is Nothing Then
         Throw New ArgumentNullException("settingID")
     Else If settingID = String.Empty Then
         Throw New ArgumentException("Setting ID Required")
     End If
    
  • here

    If settingValue IsNot Nothing Then
        Return JsonConvert.DeserializeObject(Of String())(settingValue)
    Else
        Return Nothing
    End If  
    

    the Else is redundant because if the If condition is true the Else part is nevver hit.

    If settingValue IsNot Nothing Then
        Return JsonConvert.DeserializeObject(Of String())(settingValue)
    End If
    
    Return Nothing
    

SettingLookupHelper

  • SettingLookupHelper is a noun but method names should be made out of a verb or a verb phrase. Please see: NET naming guideline

  • instead of checking against "" I would like to suggest to check against String.Empty which is IMO more readable at least if you are getting older and your eyes are getting bad you will agree.


MaterialSetting & Setting

If you are using VS 2010 you can/should use the auto-implemented properties at least if you aren't doing any validation in the setter and you don't need different acessobility modifiers (Private, Public etc.)

Your MaterialSetting would then become this

Public Class MaterialSetting
    Inherits Setting

    Public Property Material_ID As String
    Public Property Material As String

End Class

and the Setting class will look like so

Public Class Setting

Public Property Setting_ID As String
Public Property Name As String
Public Property Description As String
Public Property DataType As String
Public Property Category As String
Public Property Value As String

End Class

looks much cleaner, doesn't it ?


GetMaterialSetting

Looks good expect for the redundant Else.


GetMaterialSettingsFiltered

The String concatenation

Dim query As String
query = " SELECT * "
query += "  FROM (Materials NATURAL JOIN Material_Settings) "
query += "       NATURAL JOIN Settings "
query += queryCondition  

will create each time a new String object because strings are immutable. You should consider to use the line continuation _ like so

query = " SELECT * " _
      + "  FROM (Materials NATURAL JOIN Material_Settings) " _
      + "       NATURAL JOIN Settings " _
      + queryCondition  

You should think about wether it would be better to only include the fields you are needing into the Select instead of returning all columns. Sure you can say that you need all of them, but what will happen if you or someone else add more columns to that datatable which are needed for something else, you will still return all of them.

You are using Using that's good.
You are using a parameterized query that's good too.

You should consider to extract the actual data retrieval to a separate data access layer which shouldnÄ't be aware of any of these MaterialSetting's (or any other classes) which just returns either a DataSet or a DataTable.

In this way you could use it everywhere in your projects.

Something along these lines

Public Function GetRecords(parameters As Dictionary(Of String, Object), query As String, connectionString As String) As DataTable

    Using DataTable table As New DataTable()
        Using conn As New OracleConnection(connectionString)
            Using cmd = CreateOracleCommand(query, parameters, conn)
                Using adapter As New OracleDataAdapter(cmd)
                     adapter.Fill(table)
                     Return table
                End Using
            End Using
        End Using
    End Using
End Function  

which could then be called like so

Private Function GetMaterialSettingsFiltered(parameters As Dictionary(Of String, Object), queryCondition As String) As Settings

    Dim query As String = " SELECT * " _
                        + "  FROM (Materials NATURAL JOIN Material_Settings) " _
                        + "       NATURAL JOIN Settings " _
                        + queryCondition

    Dim settings As New Settings()

    Dim table As DataTable = GetRecords(parameters, query, GetConnectionString("WeighScaleDB"))

    If table Is Nothing Then
        Return settings
    End If

    For Each row As DataRow In table.Rows
        settings.Add(New MaterialSetting With {
                                 .Setting_ID = ConvertByteArrayToString(TryCast(row("Setting_ID"), Byte())),
                                 .Material_ID = ConvertByteArrayToString(TryCast(row("Material_ID"), Byte())),
                                 .Material = row("Material").ToString(),
                                 .Value = row("Setting_Value").ToString(),
                                 .Name = row("Setting_Name").ToString(),
                                 .Description = row("Setting_Description").ToString(),
                                 .DataType = row("Setting_Data_Type").ToString(),
                                 .Category = row("Setting_Category").ToString()
                             })
    Next

    Return settings

End Function

The name GetRecords() isn't choosen that well maybe you can come up with a better one.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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