Refactoring class methods to helper class?

Question

I have an abstract class that represents data request. The request is in XML format. There are a few classes that derive from abstract class and they do similar things.

Some basic XML operations such as create XML document, write the elements and attributes of XML document and so on are implemented as methods of abstract class and they can be used from derived classes as well.

I should refactor the abstract class. I think I could write some kind of XML helper class outside of request-based classes, but I'm not sure if it's right way.

The code looks as following:

public abstract class MWRequest
{
    public static string WebServiceURL = "url";
    protected XmlDocument xmlDoc = new XmlDocument();

    protected MWRequest()
    {
        xmlDoc.LoadXml("<?xml version=\"1.0\" encoding=\"utf-8\"?><request/>");
    }

    public XmlDocument XmlDocument
    {
        get { return xmlDoc; }
    }

    protected void PrepareRequest(string requestType)
    {
        LoadElement("protocol-version", "4.00");
        LoadElement("request-type", requestType);
        LoadExtraElement("password", password);
        LoadElement("terminal-id", terminalId);
    }

    protected void LoadElement(string elemName, string val)
    {
        XmlElement elem = xmlDoc.CreateElement(elemName);
        elem.InnerText = val;
        xmlDoc.DocumentElement.AppendChild(elem);
    }

    protected void LoadExtraElement(string name, string val)
    {
        // <extra name="ltime">60</extra>
        XmlElement elem = xmlDoc.CreateElement("extra");

        XmlAttribute attrName = xmlDoc.CreateAttribute("name");
        attrName.Value = name;
        elem.Attributes.Append(attrName);

        elem.InnerText = val;

        xmlDoc.DocumentElement.AppendChild(elem);
    }

    ...
}

Answer 1

public abstract class MWRequest
{
    public static string WebServiceURL = "url";
    protected XmlDocument xmlDoc = new XmlDocument();

    protected MWRequest()
    {
        xmlDoc.LoadXml("<?xml version=\"1.0\" encoding=\"utf-8\"?><request/>");
    }

Class name

I get the Request part of the class name, but MW could be just about anything, and itsn't descriptive. I suggest you rename the class, and drop this abbreviation.

Static field

Anything public static in an abstract class, smells. You're not showing how it's used, but being static, the value stops being an instance-level value and becomes a type-level value, which means all instances of that class will have the same WebServiceURL (which should be named WebServiceUrl - PascalCase FTW!), which just doesn't sound right, especially in a base/abstract class.

Public fields

Don't do that, you're breaking encapsulation. Derived classes can do anything they want with xmlDoc, and bypass everything you've coded as protected methods.

Encapsulation enables abstraction. Your base class could encapsulate an XmlDocument and provide an abstraction layer that hides this implementation detail to its clients and derived types. If you need to expose the XML, then override ToString and return the encapsulated XML instead of exposing the whole xmlDoc, or expose a Xml property that does that.

public XmlDocument XmlDocument
{
    get { return xmlDoc; }
}

If a derived type wants to set the XmlDocument reference, the parameterless constructor will still run and call LoadXml with the hard-coded string there. I think you should have a protected constructor that just takes an XmlDocument from the derived type, and chain the constructor calls:

protected MWRequest()
    : this(new XmlDocument())
{
    _xmlDoc.LoadXml("<?xml version=\"1.0\" encoding=\"utf-8\"?><request/>");
}

protected MWRequest(XmlDocument xmlDoc)
{
    _xmlDoc = xmlDoc;
}

private readonly XmlDocument _xmlDoc;
public XmlDocument xmlDocument { get { return _xmlDoc; } }

This way a derived type has only 1 way of setting the XmlDocument reference, through the constructor.

---

A word about LoadElement and LoadExtraElement: they're both badly named, protected and useless - a derived type can access the XmlDocument from the XmlDocument property, and do everything it needs to do with it, including destroying/replacing the entire DOM.

Either make them protected virtual and drop the XmlDocument property (and add any members to expose whatever other types need out of it), or keep the XmlDocument property and drop the helper methods - at the end of the day, it's a design decision you need to make.

If you decide to keep them, I'd suggest to change the signatures a bit:

protected virtual XmlElement CreateElement(string name, string innerText)
{
    // ...
}

protected virtual XmlElement CreateExtraElement(string name, string value)
{
    // ...
}

By making CreateElement a function that returns the XmlElement it's creating (instead of adding it itself to xmlDoc), you can delete some of the code from CreateExtraElement and call CreateElement from within CreateExtraElement

var element = CreateElement("extra", value);
var attribute = _xmlDoc.CreateAttribute("name"); // "attrName" is a bad name
attribute.Value = name;
element.Attributes.Append(attribute);
return element;

The client code is a derived class, so it can do this:

var element = CreateExtraElement("foo", "bar");
XmlDocument.DocumentElement.AppendChild(element);

And if the base class implementation for CreateExtraElement doesn't suit that class' needs, it can always override it, because it's virtual.

---

LINQ to XML

I'd suggest you use the System.Xml.Linq namespace and its XDocument and XNode, XElement, and XAttribute classes, instead of the older XmlDocument & friends classes from System.Xml.

Answer 2

When i was facing a similar task i decided to separate request/response classes from writing/parsing logic for better encapsulation. It doesnt take much time, but it allows you to change protocol or format in the future without changing your request classes.

I ended up implementing two interfaces, lets call them IMessageReader and IMessageWriter for example. Resulting classes ended up looking a lot like BinaryReader and BinaryWriter classes you might know.

In your case it might look like this:

interface IMessageWriter : IDisposable
{
    void Write(Version version);
    void Write(RequestType type);
    ...
    void Write(string element, string value);
}

abstract class MWRequest
{
    ....

    public void Write(IMessageWriter writer)
    {
        writer.Write(Version);
        writer.Write(Type);
        ...
        OnWrite(writer);
    }

    protected virtual void OnWrite(IMessageWriter writer)
    {

    }
}

Usage:

public void Send(MWRequest request)
{
    var xDoc = new XmlDocument();
    using(var writer = new XmlMessageWriter(xDoc))
    {
        request.Write(writer);
        //send xDoc somewhere
        ...
    }
}

I would also recommend using Linq to Xml instead instead of XmlDocument.