Use XmlDeserializer in factory method?

Question

I have the following class representing an XML configuration stored in the DB:

using System.IO;
using System.Xml;
using System.Xml.Serialization;

[XmlRoot("ModalConfiguration")]
public class XmlConfiguration
{
    [XmlElement("Field1")]
    public XmlConfigurationField Field1 { get; set; }
    [XmlElement("Field2")]
    public XmlConfigurationField Field2 { get; set; }
    [XmlElement("Field3")]
    public XmlConfigurationField Field3 { get; set; }

    public static XmlConfiguration GetFromXmlString(string xmlString)
    {
        using (TextReader reader = new StringReader(xmlString))
        {
            var xmlSerializer = new XmlSerializer(typeof(XmlConfiguration));
            return xmlSerializer.Deserialize(reader) as XmlConfiguration;
        }
    }

    public string ToXmlString()
    {
        var sw = new StringWriter();
        using (XmlWriter writer = XmlWriter.Create(sw))
        {
            var xmlSerializer = new XmlSerializer(typeof(XmlConfiguration));
            xmlSerializer.Serialize(writer, this);
            return sw.ToString();
        }
    }
}

A co-worker told me that this is not a good practice, especially the static factory method, since the type should not be aware of how is it built. Instead, he suggested me to create a separated class with two methods, one which takes an XML string a returns a XmlConfiguration object, and one which takes an object and returns a string; and to leave this class as a simple DTO.

However, I see no problem in the object itself knowing about the internals of its construction (by deserialization), and on the contrary I think that we achieve a nice encapsulation with my approach.

What do you think, can you point out some advantages or disadvantages of the two approaches?

Answer 1

How you approach serialization and deserialization depends on the deployment environment and the context. More specifically, how critical is this component in the overall scheme of things will affect how you approach reliability, security, testability and other ...ilities.

Naive serialization makes the assumptions that - the version of what you exported is the same as what you import. - no corruption occurs in the export format - no malicious users are targeting your DTO. If none of those are now a problem for you, then your approach might work.

Other serialization approaches might be used in the future: Java, JSON, different versions of XML, for instance, and some of them might occur in the same deployment. By separating your factory from the underlying data, you run less chance of overwhelming the underlying data with the non-trivial import/export logic.

Small projects with small requirements can roll these aspects together into the same class.

Answer 2

Your design is OK if this is the only class that is being serialized. But...

Every class that is written under this design will need GetFromXmlString and ToXmlString implementation since it is class specific.

It should be separated out so serialization can handle any class that has serialization markup. Here's an example of a generic serialization for any string to object:

        public static T XmlStringToObject<T>(this string xml, string nameSpace=null)
        {
            if (string.IsNullOrWhiteSpace(xml)) return default(T);

            var serializer = nameSpace == null ? new XmlSerializer(typeof (T)) : new XmlSerializer(typeof (T), nameSpace);

            if (!_diagnosticsOn) return (T) serializer.Deserialize(new StringReader(xml));

            serializer.UnknownNode += serializer_UnknownNode;
            serializer.UnknownElement += serializer_UnknownElement;
            serializer.UnknownAttribute += serializer_UnknownAttribute;
            return (T) serializer.Deserialize(new StringReader(xml));
        }

Now any XML string that you want to convert to an object can be done. One can also take any object and turn it into an XML string:

        public static string ToXmlString<T>(this T obj)
        {
            using (var writer = new StringWriter())
            {
                obj.ToXml(writer);
                return writer.ToString();
            }
        }

        private static void ToXml<T>(this T obj, StringWriter writer)
        {
            new XmlSerializer(typeof (T)).Serialize(writer, obj);
        }

So, this is clear separation. The original class is still decorated with the XML serialization attributes so that XML serialization can occur. If one wants to do JSON or some other format, just add those attributes (DataMember, etc) to the original class so that JSOn serialization can occur.

Again, if this is just a small project, your approach is fine, but for larger projects I would separate it out when dealing with potentially 10s to 100s of DTOs.