HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

Updating every field of every page in a form

Submitted by: @import:stackexchange-codereview··
0
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 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 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.