Extracting information and returning specific data

Question

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.

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 Select

Am I using the best coding practices and VBScript that I can be using?

Answer 1

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:

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

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:

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 If

• Malachi: unfortunately I cannot stop the flow with a goto when there is an error because it would crash the application that I am using, but I see your point. I would have to return the error through the ReturnData variable (which is assigned by the application that this script is run in) I upvote because you are right and the application I am forced to work in is wrong.
• Malachi: the script that you see is what I run on a Word document (and XML) to return certain information. so I don't really have access to the rest of the code. this is pretty much what I get to work with. I will probably be posting more of these as I have posted several in the past, just so we don't bump into the same issues and think that I am not listening, lol

Answer 2

Your faith in your input data is absolute... you do not do any validation. This is not a 'best practice'.

You should at least be doing an input range check on the oNode.

Once you have the range-check done, you can avoid the whole switch statement, and skip to some simple math for the index lookup:

' only process valid input.
If oNode >= 100 and oNode < 800 Then
 ' take advantage of the Addresses order to just do an index lookup.
 ' instead of a whole select/case statement
 ReturnData = Addresses(oNode/100 - 1)
End If

• Malachi: that is a good idea. the input can only be certain numbers, it is a controlled field in a Database that populates an XML file that is read by my Token, so there is plenty of Data Validation already done. but I see your point