0
\$\begingroup\$

I have a database, with some history data for a lot of tags (ie. measurement points in the plant).

Users supply a calculation script, and get the result of the calculation as a result. The main function looks like:

Private Function _ExecuteCalculation(calcScript As String, BeginTime As Date, EndTime As Date) As Double

    'Parse the script, and get all tag names in it
    Dim TagList As List(Of TagValues) = getTags(CalcDict, serverList)

    'query the database to populate tag values
    FillTags(BeginTime, EndTime, ListOfTags)


    Return _EvaluateCalculation(calcScript,ListOfTags)
End Function

My concern today is with the FillTags method and more globally, the TagValues class:

Public Class TagValues
    Public Name As String
    Public Average As Double
    Public Max As Double
    Public Min As Double
    Public Std As Double
    Public BeginValue As Double
    Public EndValue As Double
    Public IsError As Boolean = True

    Public ReadOnly Property DeltaV As Double
        Get
            Return EndValue - BeginValue
        End Get
    End Property

    Shared Sub FillTagsIP21(beginTime As DateTime, endTime As DateTime, TagList As List(Of TagValues))
        Dim Query As String
        Dim QueryResult As SQLData
        Dim splitArray As String(), tempTag As TagValues

        Try
            Query = String.Format(
            My.Resources.SQLQueries.TagValuesQuery,
            beginTime,
            endTime,
            String.Join("','", (From t In TagList Select t.Name).ToArray)
            )

            QueryResult = ExecuteQuery(Query)

            If QueryResult.data(0).r = "N" Then Throw New NullReferenceException("None of the tested tags exist" + String.Join(",", (From t In TagList Select t.Name).ToList))
            'Double.Parse is more robust than CDbl, which is sensitive to the servers locale with regards to decimal separator.
            For Each row In QueryResult.data.First.rows
                splitArray = Array.ConvertAll(row.fld.First.v.Split({"~#"}, StringSplitOptions.RemoveEmptyEntries), Function(a) a.Trim())
                tempTag = TagList.First(Function(a) String.Equals(a.Name.Trim(), splitArray(0).Trim(), StringComparison.OrdinalIgnoreCase))
                tempTag.Average = Double.Parse(splitArray(1), CultureInfo.InvariantCulture)
                tempTag.Min = Double.Parse(splitArray(2), CultureInfo.InvariantCulture)
                tempTag.Max = Double.Parse(splitArray(3), CultureInfo.InvariantCulture)
                tempTag.Std = Double.Parse(splitArray(4), CultureInfo.InvariantCulture)
                tempTag.BeginValue = Double.Parse(splitArray(5), CultureInfo.InvariantCulture)
                tempTag.EndValue = Double.Parse(splitArray(6), CultureInfo.InvariantCulture)
                tempTag.IsError = False
            Next
        Catch e As Exception
            Throw
        End Try
    End Sub
End Class

My main concern is with the use of a shared function (= C#'s static).

On one side, I want to query data for all tags in one query for performance reasons. On the other side, the way I have done it does not seem very object oriented.

Any improvement on both style and structure of the code are most welcome.

As a bonus question, I am trying to learn about Test-Driven development. I have understood that my unit test should not rely on a connection to the database, but I can't wrap my head around that. How can I write a function to test filling the tags with data without data?

Update: September 18th, thanks to Adriano Repetti's comments.

Parent function:

Private Function _ExecuteCalculation(calcScript As String, BeginTime As Date, EndTime As Date) As Double

        'Parse the script, and get all tag names in it
        Dim TagList As List(Of TagValues) = getTags(CalcDict, serverList)

        'query the database to populate tag values
        Dim tagListInterface = New DBReaderListOfTags(BeginTime, EndTime, ListOfTags)

        Return _EvaluateCalculation(calcScript,ListOfTags)
    End Function

With an interface:

Public Interface ITagValuesReader
    Property tagList As List(Of TagValues)
    Property beginTime As Date
    Property endTime As Date

    Function FillTags() As List(Of TagValues)
End Interface

And the implementation:

Imports System.Globalization    
Imports KPICalculationDLL.SQLData    

Public Class TagValuesReaderIP21    
    Implements ITagValuesReader    

    Public Property tagList As List(Of TagValues) Implements ITagValuesReader.tagList    
    Public Property beginTime As Date Implements ITagValuesReader.beginTime    
    Public Property endTime As Date Implements ITagValuesReader.endTime    

    Public Sub New(beginTime As Date, endTime As Date, tagList As List(Of TagValues))    
        _beginTime = beginTime    
        _endTime = endTime    
        _tagList = tagList    
    End Sub    

    Public Function FillTags() As List(Of TagValues) Implements ITagValuesReader.FillTags    
        Dim unparsedValuesArray As String()()    
        Try    
            unparsedValuesArray = GetDataFromDB()    
            For Each unparsedValuesRow In unparsedValuesArray    
                UpdateTagValuesFromDBData(unparsedValuesRow)    
            Next    
            Return tagList    
        Catch e As Exception    
            Throw    
        End Try    
    End Function    

    Private Function GetDataFromDB() As String()()    
        Dim Query As String, QueryResult As SQLData    
        Dim TimeZone As TimeZoneInfo = tagList.First.Server.TimeZone    
        Const columnSeparator As String = "~#"    


        Query = String.Format(    
            My.Resources.SQLQueries.TagValuesQuery,    
            ToSQLPlusTimestamp(beginTime, TimeZone),    
            ToSQLPlusTimestamp(endTime, TimeZone),    
            String.Join("','", (From t In tagList Select t.Name).ToArray),    
            columnSeparator    
            )    

        QueryResult = ExecuteQuery(Query, tagList.FirstOrDefault.Server.Datasource, True)    

        If QueryResult.data(0).r = "N" Then Throw New ArgumentNullException("None of the tested tags exist" + String.Join(",", (From t In tagList Select t.Name).ToList))    

        Return Array.ConvertAll(    
            QueryResult.data(0).rows,    
            Function(row) row.fld.First.v.Split({columnSeparator}, StringSplitOptions.RemoveEmptyEntries)    
            )    
    End Function    

    Private Sub UpdateTagValuesFromDBData(unparsedValuesRow As String())    
        Dim rowTagName As String, parsedFieldsArray As Double(), tempTag As TagValues    
        rowTagName = unparsedValuesRow(0).Trim()    

        'Double.Parse is more robust than CDbl, which is sensitive to the servers locale with regards to decimal separator.    
        parsedFieldsArray = Array.ConvertAll(unparsedValuesRow.Skip(1).ToArray, Function(a) Double.Parse(a, CultureInfo.InvariantCulture))    

        tempTag = tagList.First(Function(a) String.Equals(a.Name.Trim(), rowTagName, StringComparison.OrdinalIgnoreCase))    

        tempTag.Average = parsedFieldsArray(0)    
        tempTag.Min = parsedFieldsArray(1)    
        tempTag.Max = parsedFieldsArray(2)    
        tempTag.Std = parsedFieldsArray(3)    
        tempTag.BeginValue = parsedFieldsArray(4)    
        tempTag.EndValue = parsedFieldsArray(5)    
        tempTag.IsError = False    
    End Sub    
End Class    

Note: I kept String.Format, because the SQL query is not really SQL, but a SQL-ish implementation in a third-party tool that takes SQL-like strings as an input.

Is that second iteration better than the first? What can I do to further improve? Thanks a lot

\$\endgroup\$
  • \$\begingroup\$ Too rusty on VB.NET to do a review but: 1) do not throw NullReferenceException. 2) What's the point to catch and rethrow? 3) Do not compose SQL commands with String.Format() 3) Double.Parse() already handles trailing and leading spaces, no need to Trim() 4) Avoid magic constants 5) You're going through data fetched from DB and then? You just drop those results. 6) To query DB should not be a responsibility of TagValues class 7) You can use properties instead of fields. \$\endgroup\$ – Adriano Repetti Sep 14 '17 at 11:04
  • \$\begingroup\$ 8) You can extract some separate functions to make your code "breath" 9) Do not prefix function names with _ 10) You may use meaningful names. 11) How are you supposed to test FillData() without a database? 11) You may consider to store data already parsed in your DB instead of the representation used to send them to you... \$\endgroup\$ – Adriano Repetti Sep 14 '17 at 11:06
  • \$\begingroup\$ Thank you for your remarks, I have been able to make several improvements from this. However, I have a few questions about those. 5) You're going through data fetched from DB and then? You just drop those results. -> Not really, the tags in TagList have their fields filled after the function call. 6) To query DB should not be a responsibility of TagValues class -> What would be the correct way to implement this then? 8) You can extract some separate functions to make your code "breath" -> Could you please expand on this? I don't really see what I can extract. \$\endgroup\$ – Maxime Sep 18 '17 at 8:23
  • \$\begingroup\$ 10) You may use meaningful names. -> Could you please give me an example of not-meaningful name? I am knee deep in this, and, as far as I can see, everything is :( Thanks a lot :) \$\endgroup\$ – Maxime Sep 18 '17 at 8:24
  • \$\begingroup\$ Hi @Maxime, 5) you're right, I completely missed that TagList.First() part. 6) Because TagList is...a list. If you have a static method which query DB and fill results then it's pretty hard (or at least harder) to test it. You should have an interface/abstract base class TagValueReader with one implementation TagValueDbReader (for example). In this way you can test all the reader logic without an actual database and you can test DB logic without worrying about surrounding code (it's then easier to test corner cases like DB timeouts and transient errors, for example) \$\endgroup\$ – Adriano Repetti Sep 18 '17 at 8:28

Your Answer

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

Browse other questions tagged or ask your own question.