# Extract value from a database and populate objects

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?

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 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

• 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. – Adriano Repetti Sep 14 '17 at 11:04
• 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... – Adriano Repetti Sep 14 '17 at 11:06
• 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. – Maxime Sep 18 '17 at 8:23
• 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 :) – Maxime Sep 18 '17 at 8:24
• 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) – Adriano Repetti Sep 18 '17 at 8:28