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.

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.

share|improve this question

2 Answers 2

up vote 6 down vote accepted

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

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.