patternMinor
Extracting information and returning specific data
Viewed 0 times
returningextractingspecificanddatainformation
Problem
The purpose of this code is to return an address for one of the seven judicial circuits for the cases that is passed into the function.
I am taking a Node ID, which is a 3-digit number the circuit number is the first digit and the county number is the last two digits.
Am I using the best coding practices and VBScript that I can be using?
I am taking a Node ID, which is a 3-digit number the circuit number is the first digit and the county number is the last two digits.
Set oNode = XmlDoc.SelectSingleNode("/Record/CelloXml/Integration/Case/Hearing/Court/NodeID")
Dim Addresses() = (
"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip"
,"Title /n Street Address /n City, State Zip")
Select Case oNode/100
Case 1
ReturnData = Addresses(0)
Case 2
ReturnData = Addresses(1)
Case 3
ReturnData = Addresses(2)
Case 4
ReturnData = Addresses(3)
Case 5
ReturnData = Addresses(4)
Case 6
ReturnData = Addresses(5)
Case 7
ReturnData = Adresses(6)
End SelectAm I using the best coding practices and VBScript that I can be using?
Solution
Just a couple suggestions. Since VBScript isn't a strongly typed language, I would do a recommend doing more to make sure that you are making it clear what underlying types you are working on. I prefer an explicit Array call syntax for one, but that's a matter of taste more than anything:
Also make sure you declare Option Explicit (although I can't tell if it is declared or not here - this appears to be a snippet from a longer piece of code). I would also use (shudder) some sort of variable notation to let you keep track variable types. You apparently used one character Hungarian notation for your object - why not extend this to other types? Just because VBScript treats everything as a Variant doesn't mean you shouldn't keep track of how you are using them.
Next is to make sure that when you are performing casts, you are doing them explicitly. The line that jumps out is:
This is implicitly doing 2 things that are non-obvious - it takes an XmlNode object, calls its default method (.Value), and then casts it to an undefined numeric type to perform division on it. While it's fairly obvious what you are doing in this case, this is a habit that can cause all types of problems in longer scripts and can be hard to figure out when debugging or re-visiting the code in a couple months.
Speaking of debugging, I would also add error handling or trapping of some sort. Obviously trapping errors would be better than the sample below because you can give more meaningful error messages - i.e., invalid NodeID.
Finally, you can simplify this quite a bit by just indexing into the array directly:
Dim sVariable() = Array("foo","bar")Also make sure you declare Option Explicit (although I can't tell if it is declared or not here - this appears to be a snippet from a longer piece of code). I would also use (shudder) some sort of variable notation to let you keep track variable types. You apparently used one character Hungarian notation for your object - why not extend this to other types? Just because VBScript treats everything as a Variant doesn't mean you shouldn't keep track of how you are using them.
Next is to make sure that when you are performing casts, you are doing them explicitly. The line that jumps out is:
Select Case oNode/100This is implicitly doing 2 things that are non-obvious - it takes an XmlNode object, calls its default method (.Value), and then casts it to an undefined numeric type to perform division on it. While it's fairly obvious what you are doing in this case, this is a habit that can cause all types of problems in longer scripts and can be hard to figure out when debugging or re-visiting the code in a couple months.
Speaking of debugging, I would also add error handling or trapping of some sort. Obviously trapping errors would be better than the sample below because you can give more meaningful error messages - i.e., invalid NodeID.
Finally, you can simplify this quite a bit by just indexing into the array directly:
On Error Goto Ooops:
Set oNode = XmlDoc.SelectSingleNode("/Record/CelloXml/Integration/Case/Hearing/Court/NodeID")
Dim sAddresses() = Array(
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip")
Dim iIndex = CInt(oNode.Value) / 100
sReturnData = sAddresses(iIndex - 1)
Ooops:
'Fall-through is OK as long as you check for an error condition here.
If Err.Number <> 0 Then
sReturnData = vbNullString
'Do some other useful things.
End IfCode Snippets
Dim sVariable() = Array("foo","bar")Select Case oNode/100On Error Goto Ooops:
Set oNode = XmlDoc.SelectSingleNode("/Record/CelloXml/Integration/Case/Hearing/Court/NodeID")
Dim sAddresses() = Array(
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip",
"Title /n Street Address /n City, State Zip")
Dim iIndex = CInt(oNode.Value) / 100
sReturnData = sAddresses(iIndex - 1)
Ooops:
'Fall-through is OK as long as you check for an error condition here.
If Err.Number <> 0 Then
sReturnData = vbNullString
'Do some other useful things.
End IfContext
StackExchange Code Review Q#41364, answer score: 6
Revisions (0)
No revisions yet.