XML/HTML Read/Write

Question

I am developing a .Net application that involves reading data from XML and writing to HTML. The methods to do these tasks are all in the same class as the data they read/write are properties of objects of that class. The problem is that while they perform their function, they are rather long winded and complex. I would appreciate some ideas as to how I could neaten these methods up.

The class, data, and other functionality of this program ought to be irrelevant, but if requested, I'll give some more information. I just don't want this one post to get out of hand.

I'll show the methods in order of complexity according to CodeMaid/McCabe. First up is the HTML writing method at 11. This doesn't actually handle writing the HTML to a file, but returns a string with the HTML in it. About the first third of the method is "boilerplate" that does minimal work, mostly just writing HTML tags. The rest does more stuff, but I understand how someone who didn't write it could find it very confusing.

private String GetHtmlToWrite()
{
    StringWriter stringWriter = new StringWriter();
    using (HtmlTextWriter writer = new HtmlTextWriter(stringWriter))
    {
        writer.RenderBeginTag(HtmlTextWriterTag.Html);
        writer.RenderBeginTag(HtmlTextWriterTag.Head);
        writer.RenderBeginTag(HtmlTextWriterTag.Title);
        writer.Write(this.Title);
        writer.RenderEndTag();
        writer.AddAttribute(HtmlTextWriterAttribute.Href, ServerPath + ResetCssUrl);
        writer.AddAttribute(HtmlTextWriterAttribute.Type, "text/css");
        writer.AddAttribute(HtmlTextWriterAttribute.Rel, "stylesheet");
        writer.RenderBeginTag(HtmlTextWriterTag.Link);
        writer.RenderEndTag();
        writer.AddAttribute(HtmlTextWriterAttribute.Href, ServerPath + PdfCssUrl);
        writer.AddAttribute(HtmlTextWriterAttribute.Type, "text/css");
        writer.AddAttribute(HtmlTextWriterAttribute.Rel, "stylesheet");
        writer.RenderBeginTag(HtmlTextWriterTag.Link);
        writer.RenderEndTag();
        writer.RenderEndTag();
        writer.RenderBeginTag(HtmlTextWriterTag.Body);
        writer.RenderBeginTag(HtmlTextWriterTag.H1);
        writer.Write(this.Title);
        writer.RenderEndTag();
        writer.RenderBeginTag(HtmlTextWriterTag.H2);
        writer.Write("Standard Operating Procedure");
        writer.RenderEndTag();
        writer.RenderBeginTag(HtmlTextWriterTag.Table);
        writer.RenderBeginTag(HtmlTextWriterTag.Tr);

        int MetadataCount = 0;

        for (int i = 0; i < this.Data.Count; i++)
        {
            if (this.Data[i].IsMetadata)
            {
                MetadataCount++;
            }
        }

        int numRows = (2 + MetadataCount) / 3;

        for (int i = 1; i <= 3; i++)
        {
            writer.AddAttribute(HtmlTextWriterAttribute.Class, "third-width");
            writer.RenderBeginTag(HtmlTextWriterTag.Td);
            writer.AddAttribute(HtmlTextWriterAttribute.Class, "inner");
            writer.RenderBeginTag(HtmlTextWriterTag.Table);
            for (int j = (i * numRows) - numRows; j < (i * numRows); j++)
            {
                if (j < MetadataCount)
                {
                    writer.RenderBeginTag(HtmlTextWriterTag.Tr);
                    writer.RenderBeginTag(HtmlTextWriterTag.Th);
                    writer.Write(this.Data[j].Title);
                    writer.RenderEndTag();
                    writer.RenderBeginTag(HtmlTextWriterTag.Td);
                    if (!String.IsNullOrWhiteSpace(this.Data[j].Value))
                    {
                        writer.Write(this.Data[j].Value);
                    }
                    else
                    {
                        writer.Write("NA");
                    }

                    writer.RenderEndTag();
                    writer.RenderEndTag();
                }
            }
            writer.RenderEndTag();
            writer.RenderEndTag();
        }
        writer.RenderEndTag();
        writer.RenderEndTag();

        for (int i = MetadataCount; i < this.Data.Count; i++)
        {
            if (this.Data[i].Value != null)
            {
                writer.RenderBeginTag(HtmlTextWriterTag.Div);
                writer.RenderBeginTag(HtmlTextWriterTag.H3);
                writer.Write(this.Data[i].Title);
                writer.RenderEndTag();
                writer.RenderBeginTag(HtmlTextWriterTag.P);
                string[] splitContent = Regex.Split(this.Data[i].Value, @"(?<=[<.+?>])");
                foreach (string line in splitContent)
                {
                    writer.Write(line);
                }
                writer.RenderEndTag();
                writer.RenderEndTag();
            }
        }
        writer.RenderEndTag();
        writer.RenderEndTag();
    }
    return stringWriter.ToString();
}

Up next we have the first of two XML read methods. This one loads a default data template file. Its complexity is 22.

private static List<StandardOperatingProcedureData> LoadDefaultStandardOperatingProcedureData()
{
    List<StandardOperatingProcedureData> data = new List<StandardOperatingProcedureData>();
    if (File.Exists(ServerPath + StandardOperatingProcedureDataConfigPath))
    {
        using (XmlTextReader reader = new XmlTextReader(ServerPath + StandardOperatingProcedureDataConfigPath))
        {
            string ID = null;
            string title = null;
            bool? isMetadata = null;
            bool? isRequired = null;
            while (reader.Read())
            {
                if (reader.IsStartElement())
                {
                    switch (reader.Name)
                    {
                        case "ID":
                            if (reader.Read())
                            {
                                ID = reader.Value.Trim();
                            }
                            break;

                        case "Title":
                            if (reader.Read())
                            {
                                title = reader.Value.Trim();
                            }
                            break;

                        case "IsMetadata":
                            if (reader.Read())
                            {
                                switch (reader.Value.Trim().ToLower())
                                {
                                    case "false":
                                        isMetadata = false;
                                        break;

                                    case "true":
                                        isMetadata = true;
                                        break;

                                    default:
                                        throw new FileFormatException("Error: True/false incorrectly specified for field IsMetadata of data ID " + ID + ".");
                                }
                            }
                            break;

                        case "IsRequired":
                            if (reader.Read())
                            {
                                switch (reader.Value.Trim().ToLower())
                                {
                                    case "false":
                                        isRequired = false;
                                        break;

                                    case "true":
                                        isRequired = true;
                                        break;

                                    default:
                                        throw new FileFormatException("Error: True/false incorrectly specified for field IsRequired of data ID " + ID);
                                }
                            }
                            break;

                        default:
                            break;
                    }
                }
                if (!String.IsNullOrWhiteSpace(ID) && !String.IsNullOrEmpty(title) && isMetadata != null && isRequired != null)
                {
                    data.Add(new StandardOperatingProcedureData(ID, title, (bool)isMetadata, (bool)isRequired));
                    ID = null;
                    title = null;
                    isMetadata = null;
                    isRequired = null;
                }
            }
        }
    }
    else
    {
        throw new FileNotFoundException("Error: SOPData configuration file 'config/sopdata.xml' not found.");
    }
    return data;
}

Finally, we have the second XML reader with a complexity of 28. It reads from a file that actually stores data for an object of the class. Ideally, I'll eventually replace this with SQL storage, but for now, XML will have to do.

public void LoadStandardOperatingProcedure(String xmlToLoadFileName)
{
    if (File.Exists(ServerPath + SopFolder + xmlToLoadFileName))
    {
        var data = new List<StandardOperatingProcedureData>();
        using (XmlTextReader reader = new XmlTextReader(ServerPath + SopFolder + xmlToLoadFileName))
        {
            string ID = null;
            string title = null;
            bool? isMetadata = null;
            bool? isRequired = null;
            string value = null;
            while (reader.Read())
            {
                if (reader.IsStartElement())
                {
                    switch (reader.Name)
                    {
                        case "SOPTitle":
                            if (reader.Read())
                            {
                                this.Title = reader.Value.Trim();
                            }
                            break;

                        case "ID":
                            if (reader.Read())
                            {
                                ID = reader.Value.Trim();
                            }
                            break;

                        case "Title":
                            if (reader.Read())
                            {
                                title = reader.Value.Trim();
                            }
                            break;

                        case "IsMetadata":
                            if (reader.Read())
                            {
                                switch (reader.Value.Trim().ToLower())
                                {
                                    case "false":
                                        isMetadata = false;
                                        break;

                                    case "true":
                                        isMetadata = true;
                                        break;

                                    default:
                                        throw new FileFormatException("Error: True/false incorrectly specified for field IsMetadata of data ID " + ID + " in " + xmlToLoadFileName + ".");
                                }
                            }
                            break;

                        case "IsRequired":
                            if (reader.Read())
                            {
                                switch (reader.Value.Trim().ToLower())
                                {
                                    case "false":
                                        isRequired = false;
                                        break;

                                    case "true":
                                        isRequired = true;
                                        break;

                                    default:
                                        throw new FileFormatException("Error: True/false incorrectly specified for field IsRequired of data ID " + ID + " in " + xmlToLoadFileName + ".");
                                }
                            }
                            break;

                        case "Value":
                            if (reader.Read() && reader.NodeType == XmlNodeType.CDATA)
                            {
                                value = reader.Value;
                            }
                            break;

                        default:
                            break;
                    }
                }
                if (!String.IsNullOrWhiteSpace(ID) && !String.IsNullOrEmpty(title) && isMetadata != null && isRequired != null && value != null)
                {
                    data.Add(new StandardOperatingProcedureData(ID, title, (bool)isMetadata, (bool)isRequired, value));
                    ID = null;
                    title = null;
                    isMetadata = null;
                    isRequired = null;
                    value = null;
                }
            }
        }
        this.Data = data;
    }
    else
    {
        throw new FileNotFoundException("SOP file " + xmlToLoadFileName + " cannot be found.");
    }
}

It could be the case that the structure I am using for the XML files is needlessly long complex, so I am willing to discuss a restructuring/reformatting there. The default configuration file reads like this:

<SOPDataList>
  <SOPData>
    <ID>Department</ID>
    <Title>Department</Title>
    <IsMetadata>true</IsMetadata>
    <IsRequired>true</IsRequired>
  </SOPData>
  ...
</SOPDataList>

The actual data files read like this:

<StandardOperatingProcedure>
  <SOPTitle>Standard Operating Procedure</SOPTitle>
  <SOPDataList>
    <SOPData>
      <ID>Department</ID>
      <Title>Department</Title>
      <IsMetadata>True</IsMetadata>
      <IsRequired>True</IsRequired>
      <Value><![CDATA[Some Department]]></Value>
    </SOPData>
    ...
  </SOPDataList>
</StandardOperatingProcedure>

Answer 1

private String GetHtmlToWrite()

Because a StringWriter implements IDisposable it should be enclosed in a using statement too.

By extracting the writing of the default values of the HtmlWriter to a separate method the shown method will be reduced in size and will be more readable.

private void WriteDefaultValues(HtmlWriter writer)
{
    writer.RenderBeginTag(HtmlTextWriterTag.Title);
    writer.Write(this.Title);
    writer.RenderEndTag();
    writer.AddAttribute(HtmlTextWriterAttribute.Href, ServerPath + ResetCssUrl);
    writer.AddAttribute(HtmlTextWriterAttribute.Type, "text/css");
    writer.AddAttribute(HtmlTextWriterAttribute.Rel, "stylesheet");
    writer.RenderBeginTag(HtmlTextWriterTag.Link);
    writer.RenderEndTag();
    writer.AddAttribute(HtmlTextWriterAttribute.Href, ServerPath + PdfCssUrl);
    writer.AddAttribute(HtmlTextWriterAttribute.Type, "text/css");
    writer.AddAttribute(HtmlTextWriterAttribute.Rel, "stylesheet");
    writer.RenderBeginTag(HtmlTextWriterTag.Link);
    writer.RenderEndTag();
    writer.RenderEndTag();
    writer.RenderBeginTag(HtmlTextWriterTag.Body);
    writer.RenderBeginTag(HtmlTextWriterTag.H1);
    writer.Write(this.Title);
    writer.RenderEndTag();
    writer.RenderBeginTag(HtmlTextWriterTag.H2);
    writer.Write("Standard Operating Procedure");
    writer.RenderEndTag();
}  

Extracting this

    writer.RenderBeginTag(HtmlTextWriterTag.Table);
    writer.RenderBeginTag(HtmlTextWriterTag.Tr);


    int numRows = (2 + MetadataCount) / 3;

    for (int i = 1; i <= 3; i++)
    {
        writer.AddAttribute(HtmlTextWriterAttribute.Class, "third-width");
        writer.RenderBeginTag(HtmlTextWriterTag.Td);
        writer.AddAttribute(HtmlTextWriterAttribute.Class, "inner");
        writer.RenderBeginTag(HtmlTextWriterTag.Table);
        for (int j = (i * numRows) - numRows; j < (i * numRows); j++)
        {
            if (j < MetadataCount)
            {
                writer.RenderBeginTag(HtmlTextWriterTag.Tr);
                writer.RenderBeginTag(HtmlTextWriterTag.Th);
                writer.Write(this.Data[j].Title);
                writer.RenderEndTag();
                writer.RenderBeginTag(HtmlTextWriterTag.Td);
                if (!String.IsNullOrWhiteSpace(this.Data[j].Value))
                {
                    writer.Write(this.Data[j].Value);
                }
                else
                {
                    writer.Write("NA");
                }

                writer.RenderEndTag();
                writer.RenderEndTag();
            }
        }
        writer.RenderEndTag();
        writer.RenderEndTag();
    }
    writer.RenderEndTag();
    writer.RenderEndTag();

to a method WriteMetaDataTable(HtmlWriter writer, int metadataCount) will make again the code easier to read and maintain.

By using guard clauses you can reduce the vertical spacing which makes your code easier to read and to maintain.

A if..else like so

if (!condition)
{
    execute statement 1
}
else
{
    execute statement 2
}  

is always easier to read if the if condition is a positiv one like so

if (condition)
{
    execute statement 2
}
else
{
    execute statement 1
}   

Applying these points will lead to

private void WriteMetaDataTable(HtmlWriter writer, int metadataCount)
{

    writer.RenderBeginTag(HtmlTextWriterTag.Table);
    writer.RenderBeginTag(HtmlTextWriterTag.Tr);

    int numRows = (2 + metadataCount) / 3;

    for (int i = 1; i <= 3; i++)
    {
        writer.AddAttribute(HtmlTextWriterAttribute.Class, "third-width");
        writer.RenderBeginTag(HtmlTextWriterTag.Td);
        writer.AddAttribute(HtmlTextWriterAttribute.Class, "inner");
        writer.RenderBeginTag(HtmlTextWriterTag.Table);

        for (int j = (i * numRows) - numRows; j < (i * numRows); j++)
        {
            if (j >= metadataCount) { continue; }

            writer.RenderBeginTag(HtmlTextWriterTag.Tr);
            writer.RenderBeginTag(HtmlTextWriterTag.Th);
            writer.Write(this.Data[j].Title);
            writer.RenderEndTag();
            writer.RenderBeginTag(HtmlTextWriterTag.Td);

            if (String.IsNullOrWhiteSpace(this.Data[j].Value))
            {
                writer.Write("NA");                
            }
            else
            {
                writer.Write(this.Data[j].Value);
            }

            writer.RenderEndTag();
            writer.RenderEndTag();

        }

        writer.RenderEndTag();
        writer.RenderEndTag();
    }
    writer.RenderEndTag();
    writer.RenderEndTag();

}

We can also simplify the counting of the metadata items

int MetadataCount = 0;

for (int i = 0; i < this.Data.Count; i++)
{
    if (this.Data[i].IsMetadata)
    {
        MetadataCount++;
    }
}

by using the extensionmethod Count() like so

int MetadataCount = this.Data.Count(x => x.IsMetaData)

this will let the GetHtmlToWrite() method look like this

private String GetHtmlToWrite()
{
    StringWriter stringWriter = new StringWriter();
    using (HtmlTextWriter writer = new HtmlTextWriter(stringWriter))
    {
        writer.RenderBeginTag(HtmlTextWriterTag.Html);
        writer.RenderBeginTag(HtmlTextWriterTag.Head);

        WriteDefaultValues(writer);

        int metadataCount = this.Data.Count(x => x.IsMetaData)

        WriteMetaDataTable(writer, metadataCount);

        for (int i = metadataCount; i < this.Data.Count; i++)
        {
            if (this.Data[i].Value == null) { continue; }

            writer.RenderBeginTag(HtmlTextWriterTag.Div);
            writer.RenderBeginTag(HtmlTextWriterTag.H3);
            writer.Write(this.Data[i].Title);
            writer.RenderEndTag();
            writer.RenderBeginTag(HtmlTextWriterTag.P);
            string[] splitContent = Regex.Split(this.Data[i].Value, @"(?<=[<.+?>])");
            foreach (string line in splitContent)
            {
                writer.Write(line);
            }
            writer.RenderEndTag();
            writer.RenderEndTag();

        }

        writer.RenderEndTag();
        writer.RenderEndTag();
    }
    return stringWriter.ToString();
}

I would also extract the remaining loops to a separate meaningful named method but will leave it to you.

Answer 2

I would start by simplifying the ugly switch/case like this:

private static List<StandardOperatingProcedureData> LoadDefaultStandardOperatingProcedureData()
{
    // I suggest to throw an exeption right away if something's wrong
    // and not in the else block later. This way you have fewer nested blocks.
    if (!File.Exists(ServerPath + StandardOperatingProcedureDataConfigPath))
    {
        throw new FileNotFoundException("Error: SOPData configuration file 'config/sopdata.xml' not found.");
    }

    using (XmlTextReader reader = new XmlTextReader(ServerPath + StandardOperatingProcedureDataConfigPath))
    {
        string ID = null;
        string title = null;

        // This variables don't have to be nullable because if the value isn't true or false you throw an exception anyway.
        bool isMetadata;
        bool isRequired;
        while (reader.Read())
        {
            if (reader.IsStartElement())
            {
                switch (reader.Name)
                {
                case "ID": 
                    reader.IfRead(r => ID = r.Value.Trim());
                    break;

                case "Title":
                    reader.IfRead(r => title = r.Value.Trim());
                    break;

                case "IsMetadata":
                    reader.IfRead(r =>
                    {
                        if (!bool.TryParse(r.Value.Trim(), out isMetadata))
                        {
                            throw new FileFormatException("Error: True/false incorrectly specified for field IsMetadata of data ID " + ID + ".");
                        }
                    });                           
                    break;

                case "IsRequired":
                    reader.IfRead(r =>
                    {
                        if (!bool.TryParse(r.Value.Trim(), out isRequired))
                        {
                            throw new FileFormatException("Error: True/false incorrectly specified for field IsMetadata of data ID " + ID + ".");
                        }
                    });
                    break;
                }
            }

            // I would create the list where necessary and not at the top above all other code and use it later.
            List<StandardOperatingProcedureData> data = new List<StandardOperatingProcedureData>();

            // Not necessary to check isMetadata and isRequired because if the value is invalid you throw an exception
            if (!string.IsNullOrWhiteSpace(ID) && !String.IsNullOrEmpty(title))
            {
                data.Add(new StandardOperatingProcedureData(ID, title, isMetadata, isRequired));
                // You don't have to reset the values before leaving the method. They go out of scope and vanish anyway.
            }
        }
    }
    return data;
}

I like extensions that remove logic from code if this logic repeats often so I'd created an extension for it:

static class XmlTextReaderExtensions
{
    public static void IfRead(this XmlTextReader reader, Action<XmlTextReader> action)
    {
        if (reader.Read()) action(reader);
    }
}

---

You could also simplify the while and using blocks by moving the logic to somewhere else. I used an anonymous function for it:

private static List<StandardOperatingProcedureData> LoadDefaultStandardOperatingProcedureData()
{
    if (!File.Exists(ServerPath + StandardOperatingProcedureDataConfigPath))
    {
        throw new FileNotFoundException("Error: SOPData configuration file 'config/sopdata.xml' not found.");
    }

    string ID = null;
    string title = null;

    // This variables don't have to be nullable because if the value isn't true or false you throw an exception anyway.
    bool isMetadata;
    bool isRequired;

    var readProperties = new Action<XmlTextReader>(reader =>
    {
        if (!reader.IsStartElement())
        {
            return;
        }
        switch (reader.Name)
        {
        case "ID":
            reader.IfRead(r => ID = r.Value.Trim());
            break;

        case "Title":
            reader.IfRead(r => title = r.Value.Trim());
            break;

        case "IsMetadata":
            reader.IfRead(r =>
            {
                if (!bool.TryParse(r.Value.Trim(), out isMetadata))
                {
                    throw new FileFormatException(
                        "Error: True/false incorrectly specified for field IsMetadata of data ID " + ID +
                        ".");
                }
            });
            break;

        case "IsRequired":
            reader.IfRead(r =>
            {
                if (!bool.TryParse(r.Value.Trim(), out isRequired))
                {
                    throw new FileFormatException(
                        "Error: True/false incorrectly specified for field IsMetadata of data ID " + ID +
                        ".");
                }
            });
            break;
        }
    });

    using (XmlTextReader reader = new XmlTextReader(ServerPath + StandardOperatingProcedureDataConfigPath))
    {
        while (reader.Read())
        {
            readProperties(reader);
        }
    }

    List<StandardOperatingProcedureData> data = new List<StandardOperatingProcedureData>();
    // Not necessary to check isMetadata and isRequired because if the value is invalid you throw an exception
    if (!string.IsNullOrWhiteSpace(ID) && !String.IsNullOrEmpty(title))
    {
        data.Add(new StandardOperatingProcedureData(ID, title, isMetadata, isRequired));
        // You don't have to reset the values before leaving the method. They go out of scope and vanish anyway.
    }

    return data;
}