patternsqlMinor
Updating every field of every page in a form
Viewed 0 times
fieldupdatingeverypageform
Problem
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
```
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
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
Solution
I'm hoping all that's missing from your code is
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
Another golden rule, is to avoid golfed names. I don't see what's stopping
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
Throw Meaningful Exceptions
This code throws
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.
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.
Code Snippets
Throw New Exception("Could not find the identifier")Context
StackExchange Code Review Q#40042, answer score: 4
Revisions (0)
No revisions yet.