# Still pulling information from XML to insert into Word Document inside 3rd party application

I took some very good advice and changed my code around a little bit and eliminated some If statements.

I am not retrieving very much information but it looks so skinny now.

Is this a good thing? Is there something that I should add to the code?

Dim phoneNode
Dim phoneNodeList
ReturnData = ""

If phoneNodeList.Length > 0 Then
For Each phoneNode In phoneNodeList
If phoneNode.GetAttribute("ConfidentialFlag") = "True" Then
ReturnData = ReturnData & phoneNode.Getattribute("PhoneNum") & VbCrLf
End If
Next
End If


This code is very readable and simple. Is there anything that I can do to make it shorter, and should I make it shorter? it is a Script and not a fully compiled code.

-

I see one point that seems almost obvious enough that I'm worried I might be missing something. A For Each is "smart" enough that its body isn't executed at all for an empty collection, so you can eliminate the test for the list having a length of 0:

Dim phoneNode
Dim phoneNodeList
ReturnData = ""

For Each phoneNode In phoneNodeList
If phoneNode.GetAttribute("ConfidentialFlag") = "True" Then
ReturnData = ReturnData & phoneNode.Getattribute("PhoneNum") & VbCrLf
End If
Next

-
I didn't know that. thank you. –  Malachi May 15 at 13:56

You could include the conditional inside the XPath itself, and tell the XPath to select the "PhoneNum" attribute, so that phoneNodeList only contains the interesting values (multiline to prevent horizontal scrolling):

xPath = "/Record/Case/CaseFormsLoad/PartyLoad/Party/PartyPhones/Phone" & _
"[@ConfidentialFlag=""True""]/@PhoneNum"


I would avoid the string concatenations by populating an array with the attribute values:

Dim values(1 To phoneNodeList.Length)
For i = 1 To phoneNodeList.Length
values(i) = phoneNodeList(i).Text
Next


And then ReturnData can be assigned like this:

ReturnData = Join(values, VbCrLf)


This eliminates the trailing newline, and separates getting the data from outputting the result.

Resulting code:

ReturnData = ""

Dim xPath

Dim phoneNodeList
Set phoneNodeList = XmlDoc.SelectNodes(xPath)

If phoneNodeList.Length = 0 Then Exit Function 'Sub?

Dim values(1 To phoneNodeList.Length)
For i = 1 To phoneNodeList.Length
values(i) = phoneNodeList(i).Text
Next

ReturnData = Join(values, VbCrLf)


### EDIT

If the above cannot be factored into its own function, and must not Exit, then you can revert the condition and wrap the remainder in the If block:

If phoneNodeList.Length > 0 Then

Dim values(1 To phoneNodeList.Length)
For i = 1 To phoneNodeList.Length
values(i) = phoneNodeList(i).Text
Next

ReturnData = Join(values, VbCrLf)

End If

-
I am not sure that I can run this line of code If phoneNodeList.Length = 0 Then Exit Function 'Sub? this code is inserted into some other code and is run inside an application that I don't have access to underlying code. This line might crash the rest of the tokens from running on this document. I don't know what it would do, and don't want to poke this beast right now as it is working. otherwise good answer. we will see what the people say in about 20 hours and then I may accept this as the answer. –  Malachi May 15 at 18:34
@Malachi Why not extract that part into its own function? Or, (see edit), just revert the condition and increase nesting! –  Mat's Mug May 15 at 18:37