I have a simple block of legacy code that sits inside of a loop that crawls through some potentially bad xml node by node and which needs to be refactored as it is not working as intended:
Try
xmlFrag.LoadXml("<temproot>" & strXMLfragment & "</temproot>")
writer.WriteRaw(strXMLfragment)
Catch ex As Exception
InvalidXML = True
End Try
What this block is meant to do is check for valid xml and then write the xml out. What it actually does is check for invalid xml and then write the xml out only if it is valid. So it needs to be fixed to work as intended.
My first attempt at a fix:
Try
xmlFrag.LoadXml("<temproot>" & strXMLfragment & "</temproot>")
'writer.WriteRaw(strXMLfragment)
Catch ex As Exception
InvalidXML = True
Finally
writer.WriteRaw(strXMLfragment)
End Try
This works on my test data but I am concerned that WriteRaw may throw an exception on other data. I haven't found a conclusive statement about what will cause WriteRaw to throw an exception and what will happen when code in a Finally block throws an exception.
So I tried rewriting it like this:
Try
xmlFrag.LoadXml("<temproot>" & strXMLfragment & "</temproot>")
Catch ex As Exception
InvalidXML = True
End Try
Try
writer.WriteRaw(strXMLfragment)
Catch
End Try
Frankly it looks ugly as hell. Is there a more elegant way to refactor this or will the first attempt be suitable?
When an excpetion is raised in a Finally
block, nothing special happens: the exception is propagated out and up like any other exception, and code after the exception in this finally block will not be executed.
Your first attempt will fail if strXMLfragment
is null
or an empty string (or due to a already running asynchronous operation).
So if you really want to handle/swallow all exceptions, you'll have to use two Try
blocks.
To make it cleaner, you might want to pull your first Try/Catch into it's own private function and make it reusable:
Private Function TryParseXml(ByVal xml as String) as Boolean
Try
XDocument.Parse(xml)
Return True
Catch ex As Exception
Return False
End Try
End Function
Then wrap your writer.WriteRaw call in it's own Try/Catch.
Dim myXml = "<temproot>" & strXMLfragment & "</temproot>"
If TryParseXml(myXml) Then
xmlFrag.LoadXml(myXml)
Else
Try
writer.WriteRaw(strXMLfragment)
Catch ex as Exception
' handle exception
End Try
End If
Yes, ultimately this is using two Try/Catch blocks. There is no real way around this as the only real way to determine if Xml is valid is to attempt to parse it and wait for it to blow up.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With