6
\$\begingroup\$

This is code written for use inside a 3rd party application to pull information from an XML and insert into a word document, I don't know all the mechanics of the fun process of inserting the information into the word document (inside the 3rd party application) so some questions you might have for me are going to be answered with an "I don't know."

I haven't tested the code yet because it is pretty simple and I want to get changes into it before I have to shoot it to production.

Dim oNode
Dim oNodes
Set oNodes = XmlDoc.SelectNodes("/Record/Case/CaseFormsLoad/PartyLoad/Party/PartyPhones/Phone")
If oNodes.Length > 0 Then
 For Each oNode In oNodes
 If oNode.GetAttribute("ConfidentialFlag") = "True" Then
 ReturnData = ReturnData & oNode.Getattribute("PhoneNum") & VbCrLf
 End If
 If ReturnData Is Nothing Then
 ReturnData = ""
 End If
 Next
Else
 ReturnData = ""
End If

and the XML that it is parsing is in the format

<PartyPhones CurrHomeIndex="-1" CurrWorkIndex="-1" CurrCellIndex="0" CurrFaxIndex="-1" Count="2">
 <Phone PhoneID="111111" PartyID="1111111" PhoneNum="111-111-1111" PhoneExt="" PhoneTyKy="CELL" fPhoneNS="0" InvalidPhone="0" UserIDCr="101" TSCreate="3/12/2014 2:30:32 PM" UserIDChg="001" TSChange="05/01/2014 08:57:26" ConfidentialFlag="False"/>
 <Phone PhoneID="111112" PartyID="1111111" PhoneNum="111-111-1112" PhoneExt="" PhoneTyKy="CELL" fPhoneNS="0" InvalidPhone="0" UserIDCr="102" TSCreate="3/19/2014 11:10:55 AM" UserIDChg="001" TSChange="05/01/2014 08:57:26" ConfidentialFlag="True"/>
</PartyPhones>
asked May 14, 2014 at 15:17
\$\endgroup\$
2
  • \$\begingroup\$ I removed a Declaration that was redundant. \$\endgroup\$ Commented May 14, 2014 at 15:29
  • \$\begingroup\$ Follow up Question: codereview.stackexchange.com/q/49824/18427 \$\endgroup\$ Commented May 15, 2014 at 13:16

1 Answer 1

4
\$\begingroup\$

A few remarks:

Variables:

I personally like declaring my Variables with a Type (the little safety needs to be used..). And while we're at it, I don't like having lists named as items

Thus I personally(more emphasis) would rather do:

Dim oNode As Variant
Dim oNodeList As Variant

Conditionals

If ReturnData Is Nothing Then
 ReturnData = ""
EndIf

This if-statement evaluates to true only once. Instead you should initialize ReturnData before your For Each and then you can completely remove it:

Set oNodeList = 'shortcutting here
Dim ReturnData = "" As Variant/String
If oNodeList.Length > 0 Then
 ' carry on
answered May 14, 2014 at 15:25
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.