7
\$\begingroup\$

I have some VB code that connects to a MySQL database to update every control on every page of a form in our database.

I am told this is taking longer than it should to run. The 'Setup MySql connection' comment is where it starts. I am wondering if there are any obvious optimisation tricks I am missing here, or whether some things are just badly written (I wasn't sure when to open and close my MySqlDataReader and Connection objects):

    Dim penPngList As New List(Of String)

    'Get information on the pen docked
    Dim penID As String
    penID = _form.SessionData(0).DeviceState.PadDeviceID
    'penID = "aaa"

    Try
        Dim i = 0

        For Each er As ExportResult In _form.Validator.ExportResults


            If er.DataPathName = "xml" Then

                'Load the XML
                Dim doc As New XmlDocument
                doc.Load(er.ExpandedFilePath)

                'Get MySql Reader ready
                Dim rdr0 As MySqlDataReader
                Dim rdr As MySqlDataReader

                Dim sessionID As Int32

                'Grab the Descriptor
                Dim document As XPathDocument = New XPathDocument(er.ExpandedFilePath)
                Dim navigator As XPathNavigator = document.CreateNavigator()

                Dim descNode As XPathNavigator = navigator.SelectSingleNode("//MIFORMS_EXPORT/SESSION/FIELD[@NAME='DESCRIPTOR']")

                'Strip the Descriptor to get ID
                Dim descriptorString As String = descNode.InnerXml
                'Const descriptorString As String = "3_cytoxdemtest_0199_999_1"

                'Strip the Descriptor to get ID
                Dim descriptorSplitArray As String() = descriptorString.Split("_")
                Dim id As String = descriptorSplitArray(0)

                'TASK: Get the pen image PNG name
                'Get the total Session count in XML
                Dim penImageRaw As Int32 = doc.GetElementsByTagName("SESSION").Count

                If penImageRaw > 0 Then
                    'Grab the last session element (Last one is count-1)
                    Dim test As XmlNode = doc.GetElementsByTagName("SESSION").Item(penImageRaw - 1)
                    Dim list As XmlNodeList = test.ChildNodes

                    For Each node As XmlNode In list
                        If String.Equals(node.Name, "IMAGE") Then

                            Dim penImageExplode As String() = node.FirstChild.Value.Split("\")
                            PenImage = penImageExplode(penImageExplode.Length - 1)
                            penPngList.Add(PenImage)

                        End If
                    Next

                Else
                    MsgBox("ERROR: No Session nodes found")
                End If

                'Set-up MySql connection                    
                Const connStr As String = "server=localhost;user id=root; password=password; database=backend; pooling=false"
                Dim conn As New MySqlConnection(connStr)
                conn.Open()

                'Get the information from that ID the concerned ID
                Dim stm As String = "SELECT Patient , Visit, Project, Centre FROM identifiers WHERE ID = '" & id & "'"
                Dim cmd As New MySqlCommand(stm, conn)
                rdr = cmd.ExecuteReader()


                If rdr.Read Then
                    patient = rdr.GetString(0)
                    visit = rdr.GetInt32(1)
                    project = rdr.GetString(2)
                    centre = rdr.GetString(3)
                Else
                    Throw New Exception("Could not find the identifier")
                End If

                'Connect to the patient using the information gathered from above
                Dim connStrToProject As String = "server=localhost;user id=root; password=password; database=" & project & "; pooling=false"
                Dim connToProject As New MySqlConnection(connStrToProject)
                connToProject.Open()

                'Set-up MySql connection                    
                Const connStr0 As String = "server=localhost;user id=root; password=password; database=backend; pooling=false"
                Dim conn0 As New MySqlConnection(connStr0)
                conn0.Open()

                'Get the session info
                Dim stm0 As String = "SELECT MAX(Session) FROM " & project & ".bay"
                Dim cmd0 As New MySqlCommand(stm0, conn0)
                rdr0 = cmd0.ExecuteReader()

                While (rdr0.Read())
                    If rdr0("MAX(Session)") Is DBNull.Value Then
                        sessionID = 1
                    Else
                        sessionID = rdr0.GetInt32(0)
                        sessionID = sessionID + 1
                    End If
                End While

                rdr0.Close()

                'For each page of the form pages
                For Each myPage As FormPage In _form.Pages()

                    'Go through each control on the page
                    For Each myControl As FormControl In myPage.Controls()

                        Dim nowDate As String
                        nowDate = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss")

                        'convert cm to DIU (device-independent units)
                        Dim rectHeight As Int32 = (myControl.Size.Height / 2.54) * 72
                        Dim rectWidth As Int32 = (myControl.Size.Width / 2.54) * 72
                        Dim rectX As Int32 = (myControl.Position.X / 2.54) * 72
                        Dim rectY As Int32 = (myControl.Position.Y / 2.54) * 72

                        'As date variable numbers repeat, MiForm adds _x, which breaks update query. 
                        'This is where we strip that out.
                        Dim stringToCheck As String = myControl.Name
                        Const stringToFind As String = "_"
                        Dim exp As New Regex(stringToFind, RegexOptions.IgnoreCase)
                        Dim occurrences As Integer = exp.Matches(stringToCheck).Count

                        If occurrences = 1 And stringToCheck.Contains("_M") = False Or myControl.Name.Contains("_T") = False Then
                            Dim singleVal = myControl.Name.Split("_")
                            Dim sameValue = singleVal(0)

                            Dim insertQuery1 As String = "INSERT INTO audit_pen (`Session`, `Patient`, `ValidText`, `FieldNumber`, `Marker_Height`, `Marker_Width`, `Marker_X`, `Marker_Y`, `PenPng`, `Visit`, `Page`, `EntryDate`) " & _
                            "VALUES ('" & sessionID + 1 & "', '" & patient & "', '" & myControl.Value & "', '" & sameValue & "', '" & rectHeight & "', '" & rectWidth & "', '" & rectX & "', '" & rectY & "', '" & penPngList(i) & "', '" & visit & "', '" & myPage.Name & "', '" & nowDate & "')"

                            Dim insertQuery1Exe As New MySqlCommand(insertQuery1, connToProject)
                            rdr = insertQuery1Exe.ExecuteReader()
                            rdr.Close()

                            Dim insertQueryForRepeatVars As String = "UPDATE patient" & patient & " SET `Data Verified (valid)` = '1' , " & _
                            "`Valid Text (validc)` = '" & myControl.Value & "' , " & _
                            "`Verified DEC (namev)` = '" & penID & "' , " & _
                            "`Base DEC (nameb)` = '[Valid Override]' , `Compare DEC (namec)` = '[Valid Override]' , " & _
                            "`LatestPenDate` = '" & nowDate & "' , " & _
                            "`Base Entry Date (mdateb)` = '" & nowDate & "' , " & _
                            "`Compare Entry Date (mdatec)` = '" & nowDate & "' , " & _
                            "UploadedXML = '" & er.DataPathName & "' , " & _
                            "FromDigiPen = '1' , " & _
                            "rect_height = '" & rectHeight & "' , " & _
                            "rect_width = '" & rectWidth & "' , " & _
                            "rect_x = '" & rectX & "' , " & _
                            "rect_y = '" & rectY & "' , " & _
                            "PenPngLocation = '/images/" & penPngList(i) & "'" & _
                            "WHERE `Visit Number (VISIT)` = '" & visit & "' AND " & _
                            "`Page Number (PAGE)` = '" & myPage.Name & "' AND " & _
                            "`Variable Number (var)` = '" & sameValue & "'"

                            Dim insertQueryForRepeatVarsExe As New MySqlCommand(insertQueryForRepeatVars, connToProject)
                            rdr = insertQueryForRepeatVarsExe.ExecuteReader()
                            rdr.Close()

                        End If

                        'If underscore occured twice, that means "_x" was added in a date field "AO001DATE_M_x"
                        If occurrences = 2 Or myControl.Name.Contains("_M") Or myControl.Name.Contains("_T") Then
                            Dim test = myControl.Name.Split("_")
                            Dim sameVarName = test(0) + "_" + test(1)

                            Dim insertQuery2 As String = "INSERT INTO audit_pen (`Session`, `Patient`, `ValidText`, `FieldNumber`, `Marker_Height`, `Marker_Width`, `Marker_X`, `Marker_Y`, `PenPng`, `Visit`, `Page`, `EntryDate`) " & _
                            "VALUES ('" & sessionID + 1 & "', '" & patient & "', '" & myControl.Value & "', '" & sameVarName & "', '" & rectHeight & "', '" & rectWidth & "', '" & rectX & "', '" & rectY & "', '" & penPngList(i) & "', '" & visit & "', '" & myPage.Name & "', '" & nowDate & "')"

                            Dim insertQuery2Exe As New MySqlCommand(insertQuery2, connToProject)
                            rdr = insertQuery2Exe.ExecuteReader()
                            rdr.Close()

                            'Update the patient data for repeat values
                            Dim insertQueryForRepeatValues As String = "UPDATE patient" & patient & " SET `Data Verified (valid)` = '1' , " & _
                            "`Valid Text (validc)` = '" & myControl.Value & "' , " & _
                            "`Verified DEC (namev)` = '" & penID & "' , " & _
                            "`Base DEC (nameb)` = '[Valid Override]' , `Compare DEC (namec)` = '[Valid Override]' , " & _
                            "`LatestPenDate` = '" & nowDate & "' , " & _
                            "`Base Entry Date (mdateb)` = '" & nowDate & "' , " & _
                            "`Compare Entry Date (mdatec)` = '" & nowDate & "' , " & _
                            "UploadedXML = '" & er.DataPathName & "' , " & _
                            "FromDigiPen = '1' , " & _
                            "rect_height = '" & rectHeight & "' , " & _
                            "rect_width = '" & rectWidth & "' , " & _
                            "rect_x = '" & rectX & "' , " & _
                            "rect_y = '" & rectY & "' , " & _
                            "PenPngLocation = '/images/" & penPngList(i) & "'" & _
                            "WHERE `Visit Number (VISIT)` = '" & visit & "' AND " & _
                            "`Page Number (PAGE)` = '" & myPage.Name & "' AND " & _
                            "`Variable Number (var)` = '" & sameVarName & "'"

                            Dim insertQueryForRepeatValuesExe As New MySqlCommand(insertQueryForRepeatValues, connToProject)
                            rdr = insertQueryForRepeatValuesExe.ExecuteReader()
                            rdr.Close()

                        End If

                        If occurrences = 0 Then

                            Dim insertQuery2 As String = "INSERT INTO audit_pen (`Session`, `Patient`, `ValidText`, `FieldNumber`, `Marker_Height`, `Marker_Width`, `Marker_X`, `Marker_Y`, `PenPng`, `Visit`, `Page`, `EntryDate`) " & _
                            "VALUES ('" & sessionID + 1 & "', '" & patient & "', '" & myControl.Value & "', '" & myControl.Name & "', '" & rectHeight & "', '" & rectWidth & "', '" & rectX & "', '" & rectY & "', '" & penPngList(i) & "', '" & visit & "', '" & myPage.Name & "', '" & nowDate & "')"

                            Dim insertQuery2Exe As New MySqlCommand(insertQuery2, connToProject)
                            rdr = insertQuery2Exe.ExecuteReader()
                            rdr.Close()

                            'Update the patient data
                            Dim insertQuery As String = "UPDATE patient" & patient & " SET `Data Verified (valid)` = '1' , " & _
                            "`Valid Text (validc)` = '" & myControl.Value & "' , " & _
                            "`Verified DEC (namev)` = '" & penID & "' , " & _
                            "`Base DEC (nameb)` = '[Valid Override]' , `Compare DEC (namec)` = '[Valid Override]' , " & _
                            "`LatestPenDate` = '" & nowDate & "' , " & _
                            "`Base Entry Date (mdateb)` = '" & nowDate & "' , " & _
                            "`Compare Entry Date (mdatec)` = '" & nowDate & "' , " & _
                            "UploadedXML = '" & er.DataPathName & "' , " & _
                            "FromDigiPen = '1' , " & _
                            "rect_height = '" & rectHeight & "' , " & _
                            "rect_width = '" & rectWidth & "' , " & _
                            "rect_x = '" & rectX & "' , " & _
                            "rect_y = '" & rectY & "' , " & _
                            "PenPngLocation = '/images/" & penPngList(i) & "'" & _
                            "WHERE `Visit Number (VISIT)` = '" & visit & "' AND " & _
                            "`Page Number (PAGE)` = '" & myPage.Name & "' AND " & _
                            "`Variable Number (var)` = '" & myControl.Name & "'"

                            Dim insertQueryExe As New MySqlCommand(insertQuery, connToProject)
                            rdr = insertQueryExe.ExecuteReader()
                            rdr.Close()

                        End If

                    Next myControl
                    i += 1
                Next myPage
            End If
        Next

    Catch ex As Exception

    Dim nowDate As String
    nowDate = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss")

    Dim rdrError As MySqlDataReader

    'Connect to the patient using the information gathered from above
    Const connStrToProject As String = "server=localhost;user id=root; password=password; database=backend; pooling=false"
    Dim connToProject As New MySqlConnection(connStrToProject)
    connToProject.Open()

    Dim insertError As String = "INSERT INTO penerror (`Pen`, `Error`, `Study`, `ErrorDate`) VALUES ('" & penID & "', """ & ex.Message & """, '" & project & "', '" & nowDate & "')"
    Dim insertErrorExe As New MySqlCommand(insertError, connToProject)
    rdrError = insertErrorExe.ExecuteReader()
    rdrError.Close()

    End Try

Again, I am trying to optimise this for speed (obviously, it should still function as above).

\$\endgroup\$
4
\$\begingroup\$

I'm hoping all that's missing from your code is Sub DoSomething at the top and End Sub at the bottom. That's a very lengthy procedure you have here, and aside from the length itself, one sign is the "need" to append numbers to your identifiers (conn0, cmd0, rdr0, insertQuery2, ...).

Naming

Getting code to work is relatively easy. Getting code to work better is a little harder. But naming things is one of the hardest things a programmer has to do.

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

The first golden rule for good naming, is to avoid disemvoweling at all costs. An identifier called rdr is just a missed opportunity of calling it reader... and having a readable name for it (bad pun intended).

Another golden rule, is to avoid golfed names. I don't see what's stopping er from being called exportResult. Only one of the two is really meaningful.

Single Responsibility

We don't know what that procedure is called. What we do know is that the only thing it doesn't do is make you a coffee... but with so many lines of code it's possible I missed a method call that actually does it. The point here, is that you have a very procedural procedure that reads like a script. This isn't what .NET and VB.NET encourages you to do. Sure, it gets the job done, but at one point you'll want to change it, and that's when you'll start cursing at your own code.

The remedy to this headache, is single responsibility - try to break down what your code is doing, into several, smaller functions/procedures. Name each one after what it does, and make it do that - nothing more (and nothing less).

Dispose the Disposables

Your code instanciates many objects that implement the IDisposable interface, but you never call their Dispose() method. I don't know how/whether this can affect your code's performance, but cleaning up your resources can't be harmful. Wrap those (commands, connections, etc.) into Using blocks, and then you won't need to remember to Close() connections and Dispose() everything.

Throw Meaningful Exceptions

Throw New Exception("Could not find the identifier")

This code throws System.Exception - you should be throwing an exception derived from that class, either an existing one or one of your own, like UnknownIdentifierException.


I know this review doesn't address any performance issues. I think getting your code into a more maintainable shape is more important, and will allow you to better see where the optimisation opportunities are.

\$\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.