Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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

I have recently implemented a code to accomplish Object-to-String. Can you suggest any pros and cons with regard to the code?

public static string ObjectToXml(object obj)
{
    var oXmlSerializer = new XmlSerializer(obj.GetType());
    var oStringWriter = new StringWriter();
    var oXmlSerializerNamespaces = new XmlSerializerNamespaces();
    oXmlSerializerNamespaces.Add(string.Empty,string.Empty);
    var oXmlWriterSettings = new XmlWriterSettings
    {
        Indent = true,
        OmitXmlDeclaration = false,
        Encoding = Encoding.GetEncoding("ISO-8859-1")
    };

    var oXmlWriter = XmlWriter.Create(oStringWriter,oXmlWriterSettings);
    oXmlSerializer.Serialize(oXmlWriter, obj, oXmlSerializerNamespaces);
    oXmlWriter.Close();
    return oStringWriter.ToString();
}
share|improve this question
    
I definitively would not use ISO-8859-1 as the encoding. Use UTF-8 instead. It compresses as much for English but supports all languages. – Alexis Wilke Sep 27 '14 at 16:41
    
@AlexisWilke I don't think UTF-8 makes sense here. The end result is a string which is always UTF-16. – svick Sep 27 '14 at 21:58
    
@svick, the code references the wrong encoding either way... I did not realize that the string would be UTF-16 though. I do not program in C#. – Alexis Wilke Sep 27 '14 at 22:08
up vote 11 down vote accepted
  1. StringWriter and XmlWriter are both IDisposable hence their usage should be wrapped in a using statement

  2. The prefixing o of your local variables smells a bit of hungarian notation which generally does not convey a lot of useful information.

  3. The encoding has already been mentioned in the comments. In .NET strings are UTF-16 so you use should that instead.

  4. Generally try to declare your variables as close to the point of use as possible. That way it's more obvious from which point on they are being used.

Refactored code:

public static string ObjectToXml(object obj)
{
    var settings = new XmlWriterSettings
    {
        Indent = true,
        OmitXmlDeclaration = false,
        Encoding = Encoding.GetEncoding("UTF-16")
    };

    var namespaces = new XmlSerializerNamespaces();
    namespaces.Add(string.Empty,string.Empty);

    var serializer = new XmlSerializer(obj.GetType());

    using (var stringWriter = new StringWriter())
    {
        using (var xmlWriter = XmlWriter.Create(stringWriter, settings)
        {
            serializer.Serialize(xmlWriter, obj, namespaces);
        }
        return stringWriter.ToString();
    }
}

Update: As pointed out in the comments the suggested refactored code violates point 4 above. This is because I usually try to limit the scope of using blocks so it's easier to see what's going on there.

As pointed out by Jesse you could make some of the local variables class static as they are always the same. If you put generics in the mix then you could also make the serializer class static and the total refactored code could look like this:

public static class SerializerHelper<T> 
{
    private static XmlSerializer _Serializer;
    private static XmlWriterSettings _XmlSettings;

    private static XmlSerializerNamespaces _Namespaces;

    static SerializerHelper()
    {
        _Namespaces = new XmlSerializerNamespaces();
        _Namespaces.Add(string.Empty,string.Empty);

        _XmlSettings = new XmlWriterSettings
            {
                Indent = true,
                OmitXmlDeclaration = false,
                Encoding = Encoding.GetEncoding("ISO-8859-1")
            };

        _Serializer = new XmlSerializer(typeof(T));
    }

    public static string ObjectToXml(T obj)
    {
        using (var stringWriter = new StringWriter())
        {
            using (var xmlWriter = XmlWriter.Create(stringWriter, _XmlSettings))
            {
                _Serializer.Serialize(xmlWriter, obj, _Namespaces);
            }
            return stringWriter.ToString();
        }
    }
}
share|improve this answer
    
Since the settings and the namespaces aren't going to change between calls, it might prove better performance-wise to pull them out as private static readonly class-level members (obviously call namespaces.Add() from the static constructor). – Jesse C. Slicer Sep 30 '14 at 18:59
    
Didn't you "violate" point 4 ? – Heslacher Oct 1 '14 at 5:14
1  
@Heslacher yeah I noticed that after I posted. I guess it conflicts to some degree with trying to limit the scope of the using block – ChrisWue Oct 1 '14 at 6:27

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.