Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
using System.IO;
using System.Text;
using System.Xml;
using System.Xml.Serialization;

namespace Common.Extensions
{
    /// <summary>
    /// Contains the logic for streaming extensions.
    /// </summary>
    public static class StreamExtensions
    {
        /// <summary>
        /// Serialize an object.
        /// </summary>
        /// <typeparam name="T">The type of the object that gets serialized.</typeparam>
        /// <param name="stream">The stream to which the bytes will be written.</param>
        /// <param name="serializableObject">The object that gets serialized.</param>
        public static void SerializeObject<T>(this Stream stream, T serializableObject) where T : IXmlSerializable
        {
            var writerSettings = new XmlWriterSettings();

            writerSettings.Indent = true;
            writerSettings.IndentChars = "    "; // 4 spaces
            writerSettings.Encoding = Encoding.UTF8;

            using (var xmlWriter = XmlWriter.Create(stream, writerSettings))
            {
                serializableObject.WriteXml(xmlWriter);
            }

            stream.Close();
        }

        /// <summary>
        /// Deserialize a stream and return the object.
        /// </summary>
        /// <typeparam name="T">The type of the object that returns from the deserialization.</typeparam>
        /// <param name="stream">The stream which contains the bytes to deserialize.</param>
        /// <returns>The object recovered.</returns>
        public static T DeserializeObject<T>(this Stream stream) where T : IXmlSerializable
        {
            using (var xmlTextReader = XmlReader.Create(stream))
            {
                var serializer = new XmlSerializer(typeof(T));
                T result = (T)serializer.Deserialize(xmlTextReader);
                stream.Close();
                return result;
            }
        }
    }
}

I have made a class which can serialize a generic object which implements the interface IXmlSerializable and can deserialize an xml file to that same object. The idea behind this is as follows: we are using a lot of xml files to (for example) write configurations to. Since the configuration classes are often different we needed a generic function to create and read the xml files.

The stream we use (atm) is a file stream since it is currently only used for reading and writing the xml files to and from disk. Later we might use other kind of streams as well.

I am wondering if what I have written is the correct way (obviously it works but can it be made better? )

share|improve this question
up vote 4 down vote accepted
writerSettings.IndentChars = "    "; // 4 spaces  

by using

writerSettings.IndentChars = string.Empty.PadRight(4, ' ');  

you wouldn't need that noisy comment.


One wouldn't expect that a passed in Stream get closed inside that method. The closing of a Stream should only belong to the creator of that stream.


In extension methods you should always check the argument which is reffered by this if it is null.
This is because that method can also be called like so

StreamExtensions.SerializeObject<someType>(null, someObject);  

By checking (stream == null) and throwing an ArgumentNullException no unnecessary objects like XmlWriterSettings will be created.


Otherwise than the mentioned points your code looks clean and is easy to read.

share|improve this answer
    
+1 for "The closing of a Stream should only belong to the creator of that stream." – Brad M Nov 4 '15 at 16:25
    
Yes I agree on all points, only (as a development team) we decided to close the stream inside this method. Should it really not be done? Perhaps add a line in the summary of the method that the stream will be closed? – Vincent Advocaat Nov 5 '15 at 7:35
    
Maybe you(the team) should think about it again. In addition you should always assume that a public method will be used by someone else too. Having a line in the summary can be missed if it doesn't match with what one will expect. – Heslacher Nov 5 '15 at 7:56
    
Oke, we will re-discuss it, thanks for all the points :) – Vincent Advocaat Nov 5 '15 at 12:48

@Heslacher already mentioned the important stuff. One other change would be to use object initializers. I also don't see any issue with making it a private static readonly variable to avoid instantiating it every time the method is called.

      var writerSettings = new XmlWriterSettings();
      writerSettings.Indent = true;
      writerSettings.IndentChars = "    "; // 4 spaces
      writerSettings.Encoding = Encoding.UTF8;
        private static readonly XmlWriterSettings _writerSettings = new XmlWriterSettings
        {
            Indent = true,
            IndentChars = "    ",
            Encoding = Encoding.UTF8
        };
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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