Return to Answer

• 1

• 75.5k
• 18
• 194
• 467

• 75.5k
• 18
• 194
• 467

###Joined declaration & assignment

This isn't necessarily a bad use of the : instruction separator, but it did surprise me. Actually, I'd even give you a star on that one:

Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )

Pretty much any other use I can think of of the : instruction separator is hindering readability. Here I find it's a clever way of joining declaration with assignment.

However this:

Dim oNodes
Dim oNode

Set oNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent")

Should then be written like this:

Dim xPath : xPath = "Record/CelloXml/Integration/Case/ServiceEvent"
Dim ServiceEventNodes : Set ServiceEventNodes = XmlDoc.SelectNodes(xPath)

I'm not going to mention I'm allergic to things like an "o" prefix to object variables' names (oops just did), but I think you'll agree with me that ServiceEventNodes is a much more descriptive name than oNodes can ever dream to be.

I've skipped oNode here because.. I'll get there in a moment.

'''stop here and look around

I'll skip the Stop keyword and hope this isn't production code. This isn't a necessary comment, but it's well-placed - stop right here and look below, you've got If...For...If...For...If blocks nicely (!) nested here. I'd say stop here and fill up your lungs with that horrible smell, you've got two blocks of code that are rigorously identical, except for one tiny little thing; how about first deciding what the child nodes are, and then iterate them, regardless of Age.Value?

Dim ChildNodes
Select Case Age.Value
 Case "OLD":
 Set ChildNodes = ServiceEventNodes(0).childNodes
 Case "NEW":
 Set ChildNodes = ServiceEventNodes(ServiceEventNodes.length - 1).childNodes
 Case Else:
 Err.Raise 5 'invalid procedure call or argument
End Select

Notice this is not allowing the code to run any further if there's an invalid input. Returning " Hit the Else Statement " is a dangerous thing to do, because it's a valid value that your function will return just as if everything went normal, and I don't know what that's used for, but I'm sure " Hit the Else Statement " doesn't quite belong in production data.

That's why I'd rather throw an error and blow up than return a string that's actually an error message.

That being settled, I believe both loops could be completely eliminated with a little XPath (untested; I think this would do it):

Dim CommentNodes : Set CommentNodes = ChildNodes.SelectNodes("Service/Comment")

Now that you've got the nodes you're after, you can iterate them:

Dim CommentNode
For Each CommentNode In CommentNodes
 ReturnData = ReturnData & CommentNode.Text & MD
Next

I left out the & "" part, because that's concatenating an empty string, which does strictly nothing but add confusing clutter to your code.

The last part:

If Len(ReturnData) > 0 Then

 ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))

Else

Looks like it's working around the fact that you haven't properly declared and initialized ReturnData in the first place. If you put Dim ReturnData : ReturnData = vbNullString somewhere before the loop, then you don't need to assign it to an empty string if the loop resulted in a no-op.