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

Question

Follow up to This Question

---

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 = ""
Set phoneNodeList = XmlDoc.SelectNodes("/Record/Case/CaseFormsLoad/PartyLoad/Party/PartyPhones/Phone")
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.

Answer 1

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 = ""
Set phoneNodeList = XmlDoc.SelectNodes("/Record/Case/CaseFormsLoad/PartyLoad/Party/PartyPhones/Phone")
For Each phoneNode In phoneNodeList
 If phoneNode.GetAttribute("ConfidentialFlag") = "True" Then
 ReturnData = ReturnData & phoneNode.Getattribute("PhoneNum") & VbCrLf
 End If
Next

Answer 2

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
xPath = "/Record/Case/CaseFormsLoad/PartyLoad/Party/PartyPhones/Phone[@ConfidentialFlag=""True""]/@PhoneNum"
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

• Malachi: 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.
• Mathieu Guindon: @Malachi Why not extract that part into its own function? Or, (see edit), just revert the condition and increase nesting!