Custom DataContract Serializer

Question

public class InfoSerializer : IInfoSerializer
{
    private static readonly ILogger _logger = LoggerFactory.Instance.GetCurrentClassLogger();

    public string Serialize(BackgroundJobInfo info)
    {
        var stringBuilder = new StringBuilder();

        using (var stringWriter = new StringWriter(stringBuilder, CultureInfo.InvariantCulture))
        {
            var writer = new XmlTextWriter(stringWriter);
            new DataContractSerializer(typeof(BackgroundJobInfo)).WriteObject(writer, info);
        }

        return stringBuilder.ToString();
    }

    public BackgroundJobInfo Deserialize(string info)
    {
        var dataContractSerializer = new DataContractSerializer(typeof(BackgroundJobInfo));
        using (var xmlTextReader = new XmlTextReader(info, XmlNodeType.Document, new XmlParserContext(null, null, null, XmlSpace.None)))
        {
            try
            {
                var result = (BackgroundJobInfo)dataContractSerializer.ReadObject(xmlTextReader);

                return result;
            }
            catch (Exception e)
            {
                // hopefully, will never happen
                _logger.ErrorException(e, "Error when deserializing a BackgroundJobInfo object from string <{0}>", info);
                return null;
            }
        }
    }
}

Is it good serializer? Maybe I can improve it or I miss something?

Answer 1

I have a few suggestions:

1. In Deserialize() you swallow all types of exception. Catching and swallowing all exceptions is generally considered to be a bad practice. In fact, Microsoft has a compiler "design warning" for this: CA1031: Do not catch general exception types. Instead, you should swallow exceptions that you might reasonably expect, and rethrow the rest. E.g. if you were deserializing from a file, then it would be reasonable to catch and handle FileNotFoundException.

   Swallowing all exceptions can mask future problems in the code. For instance, if somebody were to modify BackgroundJobInfo and add a property whose type is not supported by the data contract serializer, or change the order of pre-existing properties thereby invalidating old XML (DataContractSerializer is order sensitive) then the resulting bug might not be immediately noticed.

   For more, see Exception Handling and/or Is catching general exceptions really a bad thing?.

2. XmlTextReader and XmlTextWriter are both deprecated as of .Net 2.0. Microsoft suggests using XmlReader.Create() and XmlWriter.Create() instead.

3. Your methods to serialize and deserialize XML from a string using DataContractSerializer look handy, and could usefully be made generic for reuse later.

Thus:

public class InfoSerializer : IInfoSerializer
{
    private static readonly ILogger _logger = LoggerFactory.Instance.GetCurrentClassLogger();

    public string Serialize(BackgroundJobInfo info)
    {
        return DataContractSerializerExtensions.SerializeXml(info, null); // If you want indentation, use new XmlWriterSettings { Indent = true, IndentChars = "    " };
    }

    public BackgroundJobInfo Deserialize(string info)
    {
        try
        {
            return DataContractSerializerExtensions.DeserializeXml<BackgroundJobInfo>(info);
        }
        catch (Exception e)
        {
            // hopefully, will never happen
            _logger.ErrorException(e, "Error when deserializing a BackgroundJobInfo object from string <{0}>", info);
            throw;
        }
    }
}

public static class DataContractSerializerExtensions
{
    public static string SerializeXml(this object obj, XmlWriterSettings settings = null)
    {
        if (obj == null)
            throw new ArgumentNullException("obj");
        using (var textWriter = new StringWriter())
        {
            using (var xmlWriter = XmlWriter.Create(textWriter, settings))
            {
                var serializer = new DataContractSerializer(obj.GetType());
                serializer.WriteObject(xmlWriter, obj);
            }
            return textWriter.ToString();
        }
    }

    public static T DeserializeXml<T>(string xml)
    {
        if (xml == null)
            throw new ArgumentNullException("xml");
        using (var textReader = new StringReader(xml))
        using (var xmlReader = XmlReader.Create(textReader))
        {
            var serializer = new DataContractSerializer(typeof(T));
            return (T)serializer.ReadObject(xmlReader);
        }
    }
}

Update

If you are using DataContractSerializer and wish to suppress the serialization of specific properties, you should apply data contract attributes, specifically [DataContract] to the type and [DataMember] to each member to be serialized. Note that data contracts are opt-in so once you start to use data contract attributes, all members of a type to be serialized must be marked as data members. For instance:

[DataContract]
public class BackgroundJobInfo
{
    [DataMember]
    public string JobName { get; set; }

    [DataMember]
    public int JobPriority { get; set; }

    // Not marked with DataMember so not serialized.
    public string Password { get; set; }
}

See Using Data Contracts.

As for creating a single static DataContractSerializer to be shared globally, I don't recommend it since there might be issues with threading. From the docs:

Thread Safety

Instances of this class are thread safe except when the instance is used with an implementation of the IDataContractSurrogate or DataContractResolver.

Unless you have determined via profiling that you have performance issues due to excessive allocations, it would appear safer to not share this class between threads.

Throwing an exception trying to serialize a property with "password" in the name is not implemented out of the box with DataContractSerializer. You should be able to adapt the XmlWriter from Custom xmlWriter to skip a certain element?, then throw an exception rather than skipping the element.