5
\$\begingroup\$

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>
\$\endgroup\$
6
  • 1
    \$\begingroup\$ For reading the XML, I would suggest taking a look into LINQ to XML. \$\endgroup\$
    – Dan Lyons
    Aug 13, 2015 at 18:25
  • \$\begingroup\$ Just out of curiosity, what namespace does HtmlWriter belong to? I was looking for something like this a little while ago. \$\endgroup\$
    – RubberDuck
    Aug 13, 2015 at 21:50
  • 1
    \$\begingroup\$ @RubberDuck you can view the documentation here. \$\endgroup\$ Aug 13, 2015 at 23:08
  • \$\begingroup\$ @DanLyons Thanks, I'll look at that. However, my eventual goal is to use SQL for storing the data. XML is a temporary solution until the organization I am coding this for gets the infrastructure they need for SQL server. \$\endgroup\$ Aug 13, 2015 at 23:11
  • \$\begingroup\$ Thanks @phantomjedi. I checked Web, but never looked at Web.UI. This looks handy. \$\endgroup\$
    – RubberDuck
    Aug 13, 2015 at 23:11

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks! Any suggestions as to the HTML write method? \$\endgroup\$ Aug 13, 2015 at 23:26
  • 1
    \$\begingroup\$ I know but still thinking ;-) \$\endgroup\$
    – t3chb0t
    Aug 14, 2015 at 4:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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