pulling specific data from XML from inside 3rd party application

Question

Another script that is used as a token to extract information from a 3rd party application, the application does other things in the background that I don't specifically know about.

This code does work inside the parent application and produces the expected output.

Is there a nicer way to write this code?

Public Function GetParameterXml()
 GetParameterXml = _
 "<Parameters>" &_
 "<Parameter Value='Age' Code='A' Description='Age' Type='Combo' Tooltip='Would you like the newest or oldest Reason?'>" &_
 " <Options>" &_
 " <Option Code='O' Description='Oldest' Value='OLD' />" &_
 " <Option Code='N' Description='Newest' Value='NEW' />" &_
 " </Options>" &_
 "</Parameter>" &_
 "</Parameters>"
End Function
'Parameter Variables
Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )
' PlaceHolder Variables
Dim oNodes
Dim oNode
Set oNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent")
'''stop here and look around
stop
If (Age.Value = "OLD") Then
 For Each iNode in oNodes(0).childNodes
 If iNode.nodeName = "Service" Then
 For Each jNode in iNode.childNodes
 If jNode.nodeName = "Comment" Then
 ReturnData = ReturnData & jNode.text & MD & ""
 End If
 Next
 End If
 Next 
 If Len(ReturnData) > 0 Then
 ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
 Else
 ReturnData = ""
 End if
ElseIf (Age.Value = "NEW") Then
 For Each iNode in oNodes(oNodes.length - 1).childNodes
 If iNode.nodeName = "Service" Then
 For Each jNode in iNode.childNodes
 If jNode.nodeName = "Comment" Then
 ReturnData = ReturnData & jNode.text & MD & ""
 End If
 Next
 End If
 Next
 If Len(ReturnData) > 0 Then
 ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
 Else
 ReturnData = ""
 End If 
Else
 ReturnData = " Hit the Else Statement "
End If

• Mathieu Guindon: What's MD? Is it declared anywhere?
• Malachi: @retailcoder MD is a Delimiter used by the application for special formatting

Answer 1

Joined declaration & assignment

This isn't necessarily a bad use of the : instruction separator, but it did surprise me. Actually, I'd even give you a star on that one:

Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )

Pretty much any other use I can think of of the : instruction separator is hindering readability. Here I find it's a clever way of joining declaration with assignment.

However this:

Dim oNodes
Dim oNode
Set oNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent")

Should then be written like this:

Dim xPath : xPath = "Record/CelloXml/Integration/Case/ServiceEvent"
Dim ServiceEventNodes : Set ServiceEventNodes = XmlDoc.SelectNodes(xPath)

I'm not going to mention I'm allergic to things like an "o" prefix to object variables' names (oops just did), but I think you'll agree with me that ServiceEventNodes is a much more descriptive name than oNodes can ever dream to be.

I've skipped oNode here because.. I'll get there in a moment.

---

'''stop here and look around

I'll skip the Stop keyword and hope this isn't production code. This isn't a necessary comment, but it's well-placed - stop right here and look below, you've got If...For...If...For...If blocks nicely (!) nested here. I'd say stop here and fill up your lungs with that horrible smell, you've got two blocks of code that are rigorously identical, except for one tiny little thing; how about first deciding what the child nodes are, and then iterate them, regardless of Age.Value?

Dim ChildNodes
Select Case Age.Value
 Case "OLD":
 Set ChildNodes = ServiceEventNodes(0).childNodes
 Case "NEW":
 Set ChildNodes = ServiceEventNodes(ServiceEventNodes.length - 1).childNodes
 Case Else:
 Err.Raise 5 'invalid procedure call or argument
End Select

Notice this is not allowing the code to run any further if there's an invalid input. Returning " Hit the Else Statement " is a dangerous thing to do, because it's a valid value that your function will return just as if everything went normal, and I don't know what that's used for, but I'm sure " Hit the Else Statement " doesn't quite belong in production data.

That's why I'd rather throw an error and blow up than return a string that's actually an error message.

---

That being settled, I believe both loops could be completely eliminated with a little XPath (untested; I think this would do it):

Dim CommentNodes : Set CommentNodes = ChildNodes.SelectNodes("Service/Comment")

Now that you've got the nodes you're after, you can iterate them:

Dim CommentNode
For Each CommentNode In CommentNodes
 ReturnData = ReturnData & CommentNode.Text & MD
Next

I left out the & "" part, because that's concatenating an empty string, which does strictly nothing but add confusing clutter to your code.

The last part:

If Len(ReturnData) > 0 Then
 ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
Else
 ReturnData = ""
End If

Looks like it's working around the fact that you haven't properly declared and initialized ReturnData in the first place. If you put Dim ReturnData : ReturnData = vbNullString somewhere before the loop, then you don't need to assign it to an empty string if the loop resulted in a no-op.

• Malachi: I like this. the reason for the weird if else stuff going on is... the structure of the XML that is being read in. there are multiple ServiceEvent Nodes with 1+ Service node where each one can have a comment, but sometimes they don't, if they do, return the text, even if there is more than one.
• Malachi: I was having a heck of a time trying to get the xPath to play nice with the VBScript on Service/Comment the way I wanted the Data. I am sure there hasn't been any testing by the Business Analysts yet, so I should be able to put this stuff into play, I will learn me to code VBScript sooner or later.
• Malachi: sorry for all the comments but I think it is relevant to the answer, this is not Production Code. and the only way to know about an exception is to have something returned via ReturnData. I work for the judicial system so this application creates forms (that are editable) and fills in the data. one line that I didn't include was the first line, on error resume next
• Malachi: ReturnData is a Variable that is created and used by the application. I don't think that I should mess with that. Code for this is like coding with my nose because I have my hands tied behind my back.
• Mathieu Guindon: @Malachi Right. ChildNodes is a IXMLDOMNodeList, you need to iterate it once. Sorry :/

Answer 2

Here is what I was able to come up with, thanks to @RetailCoder and some help on StackOverFlow Dealing with the last() function/method not working I was able to add xmlDoc.SetProperty "SelectionLanguage", "XPath" to make it work for me. so here is how I simplified the whole process.

• I replaced my If...For...If...For loops with a single switch statement and some XPath statements to grab the correct nodes
• I don't have to worry about if the Comment node exists or not, if ReturnData is empty it is taken care of by the last If statement.
• Removed the & "" like @retailcoder said

  I left out the & "" part, because that's concatenating an empty string, which does strictly nothing but add confusing clutter to your code.

• There isn't anywhere to use the Joined Declaration & Assignment, the CommentNodes variable is set differently based on the Switch statement and Declaring it in both would be redundant.
• I didn't add a Case Else the way this script functions it is not necessary to follow this path should they input a bad parameter, the application of this script is hard to describe. it is one of many tokens in a word document.
• one last thing, I only declared variables that I used in the rest of the script, the original had oNode and it was never used.
• I also didn't use any Magic iterator numbers like iNode or jNode even though I thought that was rather clever.

---

Public Function GetParameterXml()
 GetParameterXml = _
 "<Parameters>" &_
 "<Parameter Value='Age' Code='A' Description='Age' Type='Combo' Tooltip='Would you like the newest or oldest Reason?'>" &_
 " <Options>" &_
 " <Option Code='O' Description='Oldest' Value='OLD' />" &_
 " <Option Code='N' Description='Newest' Value='NEW' />" &_
 " </Options>" &_
 "</Parameter>" &_
 "</Parameters>"
End Function
'Parameter Variables
Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )
' PlaceHolder Variables
Dim CommentNodes
''' stop here and look around
stop
xmlDoc.SetProperty "SelectionLanguage", "XPath"
Select Case Age.Value
 Case "OLD":
 Set CommentNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent[1]/Service/Comment")
 Case "NEW":
 Set CommentNodes = XmlDoc.SelectNodes("/Record/CelloXml/Integration/Case/ServiceEvent[position() = last()]/Service/Comment")
End Select
For Each CommentNode In CommentNodes
 ReturnData = ReturnData & CommentNode.Text & MD
Next
If Len(ReturnData) > 0 Then
 ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
Else
 ReturnData = vbNullString 
End If 

Obviously I am going to remove the breakpoint when I put this into production. but I think this really cleaned up the code.