Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have these two methods which are generating two different xml files whose header section is same and detail section varies. The root node is different for both xml files. Here I have two methods written, one for each. I want to optimize the code by avoiding the duplication of code in these two methods. Can you please guide me achieve this?

private string GenerateType1XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{

    StringBuilder builder = new StringBuilder();
    using (StringWriter stringWriter = new StringWriter(builder))
    {
        using (XmlWriter writer = XmlWriter.Create(stringWriter))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(xmlElementConfig.CreateType1RootNode);
            WriteTypeHeader(writer, header);
            foreach (TypeDetail typ in TypeDetailList)
            {
                WriteType1Detail(writer, typ);
            }
            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    return builder.ToString();

}

private string GenerateType2XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{

    StringBuilder builder = new StringBuilder();
    using (StringWriter stringWriter = new StringWriter(builder))
    {
        using (XmlWriter writer = XmlWriter.Create(stringWriter))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(xmlElementConfig.CreateType2RootNode);
            WriteTypeHeader(writer, header);
            foreach (TypeDetail typ in TypeDetailList)
            {
                WriteType2Detail(writer, typ);
            }
            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    return builder.ToString();

}
share|improve this question

2 Answers 2

up vote 5 down vote accepted

Make a generic XML generator that receive an Action to execute your function. Something like

private string GenerateTypeXML(TypeHeader header, List<TypeDetail> TypeDetailList, string xmlRootNode Action<XmlWriter, TypeDetail> func)
{

    StringBuilder builder = new StringBuilder();
    using (StringWriter stringWriter = new StringWriter(builder))
    {
        using (XmlWriter writer = XmlWriter.Create(stringWriter))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(xmlRootNode);
            WriteTypeHeader(writer, header);
            foreach (TypeDetail typ in TypeDetailList)
            {
                func(writer, typ);
            }
            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    return builder.ToString();

}

and use it like that

private string GenerateType1XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{
    return GenerateTypeXML(header, TypeDetailList, xmlElementConfig.CreateType1RootNode,  WriteType1Detail);
}

private string GenerateType2XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{
    return GenerateTypeXML(header, TypeDetailList, xmlElementConfig.CreateType2RootNode, WriteType2Detail);
}

You might have to cast your "WriteType1Detail" to Action if it doesn't compile, I don't have visual studio right now

share|improve this answer
    
Nice! Delegates FTW! You're missing a string parameter for xmlElementConfig.CreateType1RootNode has to be xmlElementConfig.CreateType2RootNode for type-2. –  Mat's Mug Jul 2 at 17:50
    
Oops, fixed! :) –  Bruno Jul 2 at 18:13
2  
Both the solution are good and works very well, i went with secod approch of using Action since it recudes number of line of code and uses the Action feature provided. Thanks a lot for both of you guys. Bruno and Mats. –  user3651810 Jul 2 at 18:13

You need to look at what's common to both methods:

StringBuilder builder = new StringBuilder();
using (StringWriter stringWriter = new StringWriter(builder))
{
    using (XmlWriter writer = XmlWriter.Create(stringWriter))
    {
        writer.WriteStartDocument();
        writer.WriteStartElement(/* different in the two methods */);
        WriteTypeHeader(writer, header);
        foreach (TypeDetail typ in TypeDetailList)
        {
            /* different in the two methods */
        }
        writer.WriteEndElement();
        writer.WriteEndDocument();
    }
}

Looks like you could have a base class with a GenerateXml method calling into abstract members that derived types (Type1XmlGenerator, Type2XmlGenerator... whatever Type1 and Type2 means) can override:

public abstract class XmlGenerator
{
    protected abstract void WriteDetails(XmlWriter writer, TypeDetail type);
    protected abstract string XmlRootNode { get; }

    public string GenerateXml(TypeHeader header, IEnumerable<TypeDetail> details)
    {
        StringBuilder builder = new StringBuilder();
        using (StringWriter stringWriter = new StringWriter(builder))
        {
            using (XmlWriter writer = XmlWriter.Create(stringWriter))
            {
                writer.WriteStartDocument();
                writer.WriteStartElement(XmlRootNode);
                WriteTypeHeader(writer, header);
                foreach (TypeDetail detail in details)
                {
                    WriteDetails(writer, detail);
                }
                writer.WriteEndElement();
                writer.WriteEndDocument();
            }
        }
    }
}

Then you can have a class Type1XmlGenerator : XmlGenerator that will override the WriteDetails method with what you currently have in WriteType1Detail, and the XmlRootNode property getter implementation will return xmlElementConfig.CreateType1RootNode.


Now, writing/generating XML is a solved problem, doing that with a StringBuilder is error-prone. You should really look into what LINQ-to-XML (System.Xml.Linq) has in store for you.

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.