3
\$\begingroup\$

Yesterday was JSON serialization, today is XML serialization.

I've taken some of the suggestions from there and made modifications, as well as making other modifications that were not suggested but should be made.

    /// <summary>
    /// Provides methods for Serialization and Deserialization of XML/Extensible Markup Language documents.
    /// </summary>
    public class XmlSerialization
    {
        /// <summary>
        /// Serializes an object to an XML/Extensible Markup Language string.
        /// </summary>
        /// <typeparam name="T">The type of the object to serialize.</typeparam>
        /// <param name="value">The object to serialize.</param>
        /// <param name="serializedXml">Filled with a string that is the XmlSerialized object.</param>
        /// <param name="throwExceptions">If true, will throw exceptions. Otherwise, returns false on failures.</param>
        /// <returns>A boolean value indicating success.</returns>
        public static bool Serialize<T>(T value, ref string serializedXml, bool throwExceptions = false)
        {
#if DEBUG
#warning When in DEBUG Mode XML Serialization Exceptions will be thrown regardless of throwExceptions paramter.
            throwExceptions = true;
#endif

            if (value == null)
                if (throwExceptions)
                    throw new ArgumentNullException("The value is expected to be a non-null object.");
                else
                    return false;

            try
            {
                XmlSerializer xmlserializer = new XmlSerializer(typeof(T));

                using (StringWriter stringWriter = new StringWriter())
                using (XmlWriter writer = XmlWriter.Create(stringWriter))
                {
                    xmlserializer.Serialize(writer, value);

                    serializedXml = stringWriter.ToString();

                    return true;
                }
            }
            catch
            {
                if (throwExceptions)
                    throw;

                return false;
            }
        }

        /// <summary>
        /// Deserializes an XML/Extensible Markup Language string to an object.
        /// </summary>
        /// <typeparam name="T">The type of the object to serialize.</typeparam>
        /// <param name="value">The XML string representing the serialized object.</param>
        /// <param name="deserializedObject">Filled with the object that is the XmlDeserialized string.</param>
        /// <param name="throwExceptions">If true, will throw exceptions. Otherwise, returns false on failures.</param>
        /// <returns>A boolean value indicating success.</returns>
        public static bool Deserialize<T>(string value, ref T deserializedObject, bool throwExceptions = false)
        {
#if DEBUG
#warning When in DEBUG Mode XML Deserialization Exceptions will be thrown regardless of throwExceptions paramter.
            throwExceptions = true;
#endif

            if (value == null)
                if (throwExceptions)
                    throw new ArgumentNullException("The value is expected to be a non-null object.");
                else
                    return false;

            try
            {
                XmlSerializer xmlserializer = new XmlSerializer(typeof(T));

                using (StringReader stringReader = new StringReader(value))
                using (XmlReader reader = XmlReader.Create(stringReader))
                {
                    if (!xmlserializer.CanDeserialize(reader))
                            if (throwExceptions)
                                throw new ArgumentException("The provided value cannot be deserialized to the specified type.");
                            else
                                return false;

                    deserializedObject = (T)(xmlserializer.Deserialize(reader));

                    return true;
                }
            }
            catch
            {
                if (throwExceptions)
                    throw;

                return false;
            }
        }
    }

The idea is to keep it as simple as possible, and provide a common, natural form of accessing serialization.

A couple of changes:

  • I added the exception that is thrown when value is null. This wasn't there until Heslacher mentioned that I only had one spot that exceptions were thrown, as I had forgot to put it on that check as well.
  • throwErrors and all related messages/comments/documentation have been refactored to throwExceptions, errors to exceptions, et al.
\$\endgroup\$
5
\$\begingroup\$
                if (!xmlserializer.CanDeserialize(reader))
                        if (throwExceptions)
                            throw new ArgumentException("The provided value cannot be deserialized to the specified type.");
                        else
                            return false;  

I did tell you about braces {} didn't I ? Ok, I see you don't want to use them so don't whine if something goes terribly wrong.

So leaving that aside, lets talk about this snippet of code.

Because this is enclosed in a try..catch block which does check for throwExceptions there is no need to do it twice.

This can be simplified to (with braces ;-) )

if (!xmlserializer.CanDeserialize(reader))
{
    throw new ArgumentException("The provided value cannot be deserialized to the specified type.");
}   

You are using using statments for objects which implements IDisposable which is the preferred way

There isn't anything more to say but this methods are well structured, readable and also maintainable.

\$\endgroup\$
  • 1
    \$\begingroup\$ I can't help it that I'm allergic to braces...lol The only thing is, would not throwing the exception within the try-catch block cause a performance issue, since they could be discarded anyway? \$\endgroup\$ – 410_Gone Aug 14 '15 at 14:14
  • \$\begingroup\$ Maybe there is a little performance penalty but the clear advantage is that it removes code duplication hence it is easier to read and maintain. If you are afraid about performance you should profile it an check if you can live with the performance penalty or not. \$\endgroup\$ – Heslacher Aug 14 '15 at 14:18
  • \$\begingroup\$ @EBrown: I hate unnecessary braces as well. I hate it twice for simple break, return, continue and throw statements.... but having if with nested if-else without braces is really really ugly. \$\endgroup\$ – user52292 Aug 14 '15 at 15:01
  • \$\begingroup\$ @Heslacher: I would then enclose the whole method body with try..catch and have only one if(throwE..... That would be best (compiler shoud be responsible for optimization, not programmer .... unless we talk about tiny microprocessors... which we are not). \$\endgroup\$ – user52292 Aug 14 '15 at 15:06
3
\$\begingroup\$
  1. Your documentation comments are overly verbose and largely redundant in my opinion.

    • "Provides methods" - it pretty much goes without saying that it "provides methods", that's what classes are for (way more often than not).

    • "XML/Extensible Markup Language" - it's safe to assume that reader knows what XML is. And if they don't, expanding the acronym isn't likely to help.

    • "<returns>A boolean value" - I can see that it returns a boolean value. Why skip the information for throwExceptions? The comment currently sums up: "If true, will throw exceptions...". By the same logic it should rather read: "A boolean value which, if equal to true..." ;)

  2. I'm not sure if it's such a hot idea to overwrite a parameter in debug mode (with your #if DEBUG directive), even if false was passed explicitly. It's sort of overdoing it, and probably at odds with the principle of least astonishment.

  3. You know what type that is, so you could include both value.GetType() as well as typeof(T) in the exception message to make it more informative.

    throw new ArgumentException("The provided value cannot be deserialized to the specified type.");
    
  4. (Some) documentation comments for your Deserialize method still talk about serializing instead of deserializing (copy-pasta).

  5. Why make these methods static? Now it can't be subclassed, you can't create an interface out of it, you can't use dependency injection to pass this serializer around, you can't mock it. It will be tight coupling wherever you use it. It's falling back to the days of procedural programming. Note that one can always create singletons or static wrappers around a normal object if they need it.

\$\endgroup\$
2
\$\begingroup\$

Use braces for non-trivial statements inside conditional or loop

if (value == null)
{
    if (throwExceptions)
        throw new ArgumentNullException("The value is expected to be a non-null object.");
    else
        return false;
}

I am fine with no-braces around return, break, continue and throw. Alternative would be:

if (value == null)
{
    if (throwExceptions)
        throw new ArgumentNullException("The value is expected to be a non-null object.");
    return false;
}

as the else is unnecessary

Reuse code, don't duplicate

You can use one try...catch and have one if (throwExceptions) in the final catch for each method body. That could possibly have some speed penalty, but not something really important. Simplicity, readability and maintainability is more important (especially for managed language/code).

Why the bool argument?

Do you really need it? Does it have to be parameter? Are you calling it with variable or always true, false or default?

I would personaly create methods returning void that throws (e.g. void Serialize... and one with name starting with Try (e.g. bool TrySerialize...) that would call the first and return false on exception.

Actually, you intent to throw for DEBUG, to know it, to diagnose. So, it seems to be designed to throw, not to return false (this sounds like: we rather remain silent in release instead of crashing the app).

\$\endgroup\$
  • 1
    \$\begingroup\$ Spot on with the bool argument. I use it because I never want exceptions suppressed directly within this method in DEBUG mode. \$\endgroup\$ – 410_Gone Aug 14 '15 at 15:51
  • 1
    \$\begingroup\$ Do you ever call it with argument or always use the default? \$\endgroup\$ – user52292 Aug 14 '15 at 15:55
  • 1
    \$\begingroup\$ It depends on where/what it's serializing. There's a lot of different data it works with, so for example, anytime it serializes or deserializes a players Account information I call it with that parameter. \$\endgroup\$ – 410_Gone Aug 14 '15 at 15:56
  • 1
    \$\begingroup\$ With variable or constant? Would my solution with TrySerialize not throwing and Serialize throwing work for you? \$\endgroup\$ – user52292 Aug 14 '15 at 15:57
  • 2
    \$\begingroup\$ If he went with a Serialize/TrySerialize split, I would follow the framework convention of returning the string for Serialize and use an out parameter for it in TrySerialize. Using a single ref/out parameter in a void method is unnecessary and a little confusing. \$\endgroup\$ – Dan Lyons Aug 14 '15 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.