11
\$\begingroup\$

Lately I have been learning about serialization so I decided to write a little helper class for my application to make it easy to use this feature in multiple places. I actually mixed some generics in the code and want to hear your feedback on my overall approach.

SerializationService.cs

public class SerializationService
{
    /// <summary>
    /// This method Serializes the given Obj into an XML file.
    /// </summary>
    /// <param name="myObject">The obj to be serialized.</param>
    /// <param name="fileFullPath">The file name with the full
    /// path for the output serialized XML file.</param>
    public static void SerializeToFile(Object myObject, string fileFullPath)
    {
        var xmlDoc = new XmlDocument();
        var xmlSerializer = new XmlSerializer(myObject.GetType());
        using (var fs = new FileStream(fileFullPath, FileMode.Create))
        {
            var writer = new XmlTextWriter(fs, Encoding.Unicode);
            xmlSerializer.Serialize(writer, myObject);
            writer.Close();
        }
    }

    /// <summary>
    /// This method deserializes the XML file into an Obj.
    /// </summary>
    /// <param name="fuleFullPath">The XML file to read.</param>
    /// <returns>Deserialized obj of a type T.</returns>
    public static T DeserializeToObj<T>(string fuleFullPath)
    {
        var xmlSerializer = new XmlSerializer(typeof(T));
        using (var reader = new FileStream(fuleFullPath, FileMode.Open))
        {
            return (T) xmlSerializer.Deserialize(reader);
        }
    }
}

And here is how I use it in my code:

Serialization

SerializationService.SerializeToFile(
    _partRepository.GetParts().Where(p => p.IsIgnored).ToList(),
    fileFullPath);

De-serialization

var ignoredParts = SerializationService.DeserializeToObj<List<Part>>(ignoredPartsXml);
\$\endgroup\$
  • \$\begingroup\$ @RubberDuck, here is the little helper class uploaded as a question as promised. Would appreciate to hear you input on this. Thanks \$\endgroup\$ – Mehrad Jan 21 '15 at 6:10
  • 1
    \$\begingroup\$ You've gotten some great answers already. Sorry I'm late to the party. Question for you to consider. How will you be using this in your project? Will you be writing many objects to many files, or will you be writing one object containing a bunch of objects to one file? If it's the latter, don't bother with the generics. If it the prior, be careful that you don't try to write two different objects to the same file. It doesn't append, but overwrites the contents of the file. \$\endgroup\$ – RubberDuck Jan 21 '15 at 10:47
11
\$\begingroup\$

Here are a few remarks. I'm not perfect or write perfect code but I hope these tips will help write better code:

Naming consistency:

It would be easier to understand if the names of your two methods had opposing names. For example: SerializeToFile and DeserializeFromFile. It's obvious that deserialization will return an object, omit that in the name.

Also, if you're using the same type/kind of parameter or variable in one method, give it the same name in the other method.

Truly generic:

You should make your serialize method generic too:

public static void SerializeToFile<T>(T obj, string fullPath)

In the point Type constraint a bit further you'll see why.

Scoping variables:

You should define variables as close to their usage as possible. In your case you can instantiate the XmlSerializer class inside the using statement since you don't need it outside the statement. Example:

using (var stream = new FileStream(fullPath, FileMode.Open))
{
    var xmlSerializer = new XmlSerializer(typeof(T));
}

Checking for the file:

In both methods an exception can be thrown in certain situations. In the serialize method, if the path doesn't exist you'll get a DirectoryNotFoundException and in the deserialize method you'll get a FileNotFoundException if the file doesn't exist. This can easily be solved:

//Check for directory in serialize:
var directoryPath = Path.GetDirectory(fileFullPath);
var directoryExists = Directory.Exists(directoryPath);

//Check for file in deserialize
var fileExists = File.Exists(fileFullPath);

You should decide whether you want positive or negative evaluation on this. For example, only perform the method if the checks are positive or throw an argument exception if the checks are negative. That is up to you.

Also, like Heslacher stated: what if the file already exists? As for now, the content of the file will be overwritten by the new content.

using statement:

Following:

using (var fs = new FileStream(fileFullPath, FileMode.Create))
{
    var writer = new XmlTextWriter(fs, Encoding.Unicode);
    xmlSerializer.Serialize(writer, myObject);
    writer.Close();
}

can be replaced by:

using (var stream = new FileStream(fullPath, FileMode.Create))
using (var writer = new XmlTextWriter(stream, Encoding.Unicode))
{
    var xmlSerializer = new XmlSerializer(obj.GetType());
    xmlSerializer.Serialize(writer, obj);
}

Type constraint:

Remember my point for making the serialize method generic too? For now, you don't validate the incoming and outgoing object. What if I pass an instance of a class that has no public parameterless constructor? During runtime I'll get an InvalidOperationException with a message like:

... cannot be serialized because it does not have a parameterless constructor.

The solution is to place type constraints on your methods. Definition from MSDN:

When you define a generic class, you can apply restrictions to the kinds of types that client code can use for type arguments when it instantiates your class, these restrictions are called constraints.

You should use the class and new() type constraints. Here's their definition:

  • class: the type argument must be a reference type; this applies also to any class, interface, delegate, or array type.

  • new(): the type argument must have a public parameterless constructor. When used together with other constraints, the new() constraint must be specified last.

Now you'll get a compile-time exception when trying to serialize an instance of a clas without a public parameterless constructor. You can read more on this here: Constraints on Type Parameters (C# Programming Guide).

public static void SerializeToFile<T>(T obj, string fullPath) where T : class, new()
{
    //method body
}

public static T DeserializeFromFile<T>(string fullPath) where T : class, new()
{
    //method body
}

Final code:

With all this applied, your code might look like following:

public class SerializationService
{
    public static void SerializeToFile<T>(T obj, string fullPath) where T : class, new()
    {
        if(Directory.Exists(Path.GetDirectoryName(fullPath)))
        {
            using (var stream = new FileStream(fullPath, FileMode.Create))
            using (var writer = new XmlTextWriter(stream, Encoding.Unicode))
            {
                var xmlSerializer = new XmlSerializer(obj.GetType());
                xmlSerializer.Serialize(writer, obj);
            }
        }
    }

    public static T DeserializeFromFile<T>(string fullPath) where T : class, new()
    {
        if(File.Exists(fullPath))
        {
            using (var stream = new FileStream(fullPath, FileMode.Open))
            {
                var xmlSerializer = new XmlSerializer(typeof(T));
                return (T) xmlSerializer.Deserialize(stream);
            }
        }
        return default(T);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ OP may or may not want to test for the existence of the directory in the serialize method. If the directory doesn't exist, he may want to catch the exception so it can be built or, more properly, test its existence outside of this method and build it if it doesn't exist. It depends on the use case. \$\endgroup\$ – RubberDuck Jan 21 '15 at 12:38
  • 1
    \$\begingroup\$ You're absolutely right. I stated this in other words in my answer, except for the outside-the-method-testing. :) \$\endgroup\$ – Abbas Jan 21 '15 at 12:42
  • 2
    \$\begingroup\$ Ahh. You're right. I missed that. (Great answer by the way.) \$\endgroup\$ – RubberDuck Jan 21 '15 at 12:50
  • \$\begingroup\$ Restricting to class is needlessly restrictive. De/serializing value types works perfectly fine. \$\endgroup\$ – ChrisWue Jan 21 '15 at 18:34
7
\$\begingroup\$
  • string fuleFullPath will be a mistake.
  • you should check at DeserializeToObj if the file exists. If it doesn't you should throw an exception.
  • you should think about your strategie for SerializeToFile what you want to do, if the file already exists.
  • var xmlDoc = new XmlDocument(); isn't doing anything and should be removed.
  • you should consider to rename the DeserializeToObj() method to just Deserialize(). It is obvious that it deserialize to an object.
  • you should be consistent with the naming. In the SerializeToFile() method you name the FileStream object fs in the DeserializeToObj() method it is called reader.
\$\endgroup\$
  • \$\begingroup\$ Since Deserialize is an original .NET method I though I will alter the name to prevent conflicts and mistakes. Would this be a right approach? \$\endgroup\$ – Mehrad Jan 21 '15 at 6:45
  • 2
    \$\begingroup\$ I don't see a problem with using the same names, because you are using a different classname and namespace. \$\endgroup\$ – Heslacher Jan 21 '15 at 6:50
  • \$\begingroup\$ Also, will it be better to wrap var writer in a using ? \$\endgroup\$ – janos Jan 21 '15 at 6:56
  • 1
    \$\begingroup\$ @janos, I have thought about this too, but some StyleCop will maybe complain about disposing disposed object, because using a using for writer too, will dispose the underlaying stream too. \$\endgroup\$ – Heslacher Jan 21 '15 at 6:59
5
\$\begingroup\$
  1. SerializeToFile should be generic as well. Seems odd that the serialization method works on Object while the deserialization method is generic.

  2. I'd consider adding methods which de/serialize from/to streams. While you have effectively applied YAGNI, writing/reading to/from stream is something I encounter quite often once the projects grow beyond basic experimentation.

\$\endgroup\$

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.