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.

asked Nov 5, 2015 at 17:23
\$\endgroup\$
2
  • \$\begingroup\$ May I ask why you're not using Encoding.GetString? \$\endgroup\$ Commented Nov 5, 2015 at 18:13
  • \$\begingroup\$ @Bjørn-RogerKringsjå Thanks for the tip. I didn't know about that function. \$\endgroup\$ Commented Nov 5, 2015 at 19:13

1 Answer 1

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.

answered Nov 6, 2015 at 7:29
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.