Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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>
share|improve this question
    
I removed a Declaration that was redundant. –  Malachi May 14 at 15:29
    
Follow up Question: codereview.stackexchange.com/q/49824/18427 –  Malachi May 15 at 13:16

1 Answer 1

up vote 4 down vote accepted

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.