patterncsharpMinor

XML/HTML Read/Write

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

writehtmlxmlread

Problem

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);

Solution

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

private static List 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 data = new List();

            // 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 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 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(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())
{
read

Code Snippets

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;
}

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

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;
}

Context

StackExchange Code Review Q#100824, answer score: 4

Revisions (0)

No revisions yet.