Refactoring XmlWriter
I inherited some code that make heavy use of XmlWriter. Say what you will about XmlWriter, it works, but the code using it can be ugly, hard to read, and hard to figure out where you went wrong. Case in point, there were numerous errors in the code that went unfound by the unit test, because the XmlWriter somehow rights itself. The culprit was missing WriteEndElement calls.
To keep things correct, you really need to have a WriteEndElement for every WriteStartElement, or risk future problems. This went undetected because when the project started the xml was simple, just a couple of elements, then call ToString(). As soon as you call ToString, the XmlWriter adds the nessesary end elements into the document.
Once I started looking into this, it was easy to see how this could happen. And I wanted to make it easier to NOT happen. So here is the code I started with:
1: protected void WriteInvoiceLineToAdd(XmlWriter r, OrderItem orderItem)
2: {
3: r.WriteStartElement("InvoiceLineAdd");
4: r.WriteRefNode("ItemRef", "FullName", orderItem.ProductName);
5: r.WriteElementString("Quantity", orderItem.Quantity);
6: r.WriteElementString("Rate", orderItem.Rate);
7: r.WriteEndElement();
8: }
My first refactoring was simply to add some artificial scoping:
1: protected void WriteInvoiceLineToAdd(XmlWriter r, OrderItem orderItem)
2: {
3: r.WriteStartElement("InvoiceLineAdd");
4: {
5: r.WriteRefNode("ItemRef", "FullName", orderItem.ProductName);
6: r.WriteElementString("Quantity", orderItem.Quantity);
7: r.WriteElementString("Rate", orderItem.Rate);
8: }
9: r.WriteEndElement();
10: }
Which actually worked fairly well. Unfortunately, this still left the issue open (missing EndElement calls). I was able to find the areas where I needed to fix, but it wasn’t what I considered solid yet.
Which means, back to Extension Methods and the Action delegate. I created the following Extension Methods which overload the WriteStartElement:
1: public static void WriteStartElement(this XmlWriter xe, string localName, Action action)
2: {
3: xe.WriteStartElement(localName);
4:
5: action.Invoke();
6:
7: xe.WriteEndElement();
8: }
9:
10: public static void WriteStartElement(this XmlWriter xe, string localName, Action<XmlWriter> action)
11: {
12: xe.WriteStartElement(localName);
13:
14: action.Invoke(xe);
15:
16: xe.WriteEndElement();
17: }
Which now allow me to remove the WriteEndElement altogether and refactor to this instead:
1: protected void WriteInvoiceLineToAdd(XmlWriter r, OrderItem orderItem)
2: {
3: r.WriteStartElement("InvoiceLineAdd", () => {
4: r.WriteRefNode("ItemRef", "FullName", orderItem.ProductName);
5: r.WriteElementString("Quantity", orderItem.Quantity);
6: r.WriteElementString("Rate", orderItem.Rate);
7: });
8: }
No more missing the WriteEndElement. No having to parse the code to see where the end element should have gone.
Also note, I created two overloads for WriteStartElement, the other allows you to write code like this:
1: protected void WriteInvoiceLineToAdd(XmlWriter r, OrderItem orderItem)
2: {
3: r.WriteStartElement("InvoiceLineAdd", x => {
4: x.WriteRefNode("ItemRef", "FullName", orderItem.ProductName);
5: x.WriteElementString("Quantity", orderItem.Quantity);
6: x.WriteElementString("Rate", orderItem.Rate);
7: });
8: }
I like this refactoring but I think the name of your extension method should be changed.
WriteStartElement is not what final routine does. It does a lot more than just write the first element.
It actually writes a _chunk_ of XML encapuslated with the tag given in the first argument to the routine.
I’d name it something like WriteChunk or WriteBlock.
Hi Simon,
I am not disagreeing with you. I left the name the same to make the refactoring process easier.
The next step, once the initial refactoring is done, should be to rename the method to something like what you suggest (that is the beauty of refactoring tools, renaming a method is now trivially easy).
I took a stab at this problem some time ago too: http://devlicio.us/blogs/sergio_pereira/archive/2008/04/14/designing-with-lambdas-part-ii.aspx
Hey Sergio,
Now that you bring it up I remember you posting that. I had to laugh/cringe at some of the comments you go for that tho.
I’m still waiting to see if the anti-lambda police will come after me for this one as well.
You’re sort of recreating the syntax used by LINQ to XML, was that not available for your project?
@John, Inherited code. You make it work for you.
How about implementing IDisposable and writing the end element in the dispose?
@John, I’ve seen Rhino Mocks use the ‘using’ statement to give a type of closure for doing things like that. The nice part about using a ‘using’ statement is the code is then .net 2.0 compatible.
But, I can’t just implement IDisposable on XmlWriter. More than likely it already implements it anyway, which means I don’t think I can overwrite it. That means I would still have to create a new method for XmlWriter that returned a custom object (which implements IDisposable) so I could use ‘using’ on that object — and then I would have to re-implement a whole bunch of XmlWriter methods.
Basically, unless you own the entire code base, and IDisposable is not already in use, … no that is just a lot of work.
What about performance? from my VERY non-complete testing, the new solution takes almost 3 times longer to execute.
Obviously, if you are creating small XMLs this is not an issue, but if you have to handle a large data set, aren’t you worried that using the Action delegate hurts performance too much?
1. were you testing in debug mode? That changes a lot of metrics.
2. a delegate is nothing more than a function pointer. I use them constantly in a multitude of situations and have never seen a significant performance hit using delegates. Note: Action and Func are still only delegates.
3. Define large and I’ll tell you if I care.
4. In my case, the database calls make up the VAST majority of the performance problems.