Geeks With Blogs

News All source code published on this blog is placed in the public domain.
Chris Falter .NET Design and Best Practices

If you're not living DRY, you're not a good programmer.

Okay, I'm not talking about alcohol consumption!  I'm talking about the Don't Repeat Yourself principle--a principle every programmer should live by.  Recently I did a code review for a project we outsourced, and demonstrated how you can refactor bad cut-and-paste code into well-designed code.  You will note that the capabilities of the MagicStringTranslator class really help to reduce code clutter. Without further ado, I present the relevant portion of my code review:

...your branching logic has 12 lines of code in each branch except the final one.  I am inserting an excerpt for reference:

        if (m_record.Text.Substring(13, 6).Trim() == "IDENT")
        {
            writer.WriteBreak();
            writer.WriteFullBeginTag("Table style='width: 100%' border='1'");
                writer.WriteFullBeginTag("tr style='background-color:#ccffcc;'");
                    writer.WriteFullBeginTag("td style='width: 100%;' colspan='2' align='center'");
                        writer.WriteFullBeginTag("span style='font-size: 9pt;");
                        writer.WriteFullBeginTag("strong");
                        writer.Write("SUBJECT INFORMATION");
                        writer.WriteEndTag("strong");
                        writer.WriteEndTag("span");
                    writer.WriteEndTag("TD");
                writer.WriteEndTag("TR");
            writer.WriteEndTag("Table");
        }
        if (m_record.Text.Substring(13, 6).Trim() == "SUMRY")
        {
            writer.WriteBreak();
            writer.WriteFullBeginTag("Table style='width: 100%' border='1'");
                writer.WriteFullBeginTag("tr style='background-color:#ccffcc;'");
                    writer.WriteFullBeginTag("td style='width: 100%;' colspan='2' align='center'");
                        writer.WriteFullBeginTag("span style='font-size: 9pt;");
                        writer.WriteFullBeginTag("strong");
                        writer.Write("SUMMARY");
                        writer.WriteEndTag("strong");
                        writer.WriteEndTag("span");
                    writer.WriteEndTag("TD");
                writer.WriteEndTag("TR");
            writer.WriteEndTag("Table");
        }
        if (m_record.Text.Substring(13, 6).Trim() == "COLLT")
        {
           // this code and several more near-identical branches are omitted for brevity’s sake
        }

Eleven of these lines of code are identical.  This is bad for 3 reasons:

  1. What would happen if one of the lines of code had a bug?  You would have to fix it in each branch, and be sure to do it the same way in each one.  This code structure is bug-prone.
  2. The structure implies that each branch does different things, when the code in fact does essentially the same thing.  As a result, the code structure is misleading.
  3. The code ends up being way too verbose; it is saying the same thing over and over and over.  As a result, the code structure is hard to read and understand.

In fact, your code does suffer from an insidious little bug.  You are comparing a substring of length 6 to a literal of length 5.

        if (m_record.Text.Substring(13, 6).Trim() == "IDENT")

This only happens to work when the 6th character is a blank, because in this circumstance the Trim() method reduces the string to length 5, and the comparison is valid.  What would happen if the 6th character did not happen to be a blank?

But don't try to fix this bug in the seven places where it occurs!  There are better ways to improve the code.  One way is to restrict the branching logic to the one line of code, then use that one line together with the other 11 exactly once.

        string title = String.Empty;
 
        switch (m_record.Identifier)
        {
            case "IDENT":
                title = "SUBJECT INFORMATION";
                break;
            case "SUMRY":
                title = "SUMMARY";
                break;
            case "COLLT":
                title = "COLLECTION ITEMS";
                break;
            case "ACCNT":
                title = "TRADE ACCOUNT ACTIVITY";
                break;
            case "MSSGE":
                title = "MESSAGE";
                break;
            case "EMPLY":
                title = "EMPLOYMENT";
                break;
            case "INQHS":
                title = "INQUIRY HISTORY";
                break;
        }
 
        writer.WriteFullBeginTag("Table style='width: 100%' border='1'");
            writer.WriteFullBeginTag("tr style='background-color:#ccffcc;'");
                writer.WriteFullBeginTag("td style='width: 100%;' colspan='2' align='center'");
                    writer.WriteFullBeginTag("span style='font-size: 9pt;");
                    writer.WriteFullBeginTag("strong");
                    writer.Write(title);
                    writer.WriteEndTag("strong");
                    writer.WriteEndTag("span");
                writer.WriteEndTag("TD");
            writer.WriteEndTag("TR");
        writer.WriteEndTag("Table");
 
An even better way is to delegate the relationship between magic string (e.g., “INQHS”) and description (e.g., “INQUIRY HISTORY”) to a class specialized for this purpose.  Then you can simply use the class directly, without writing any branching logic at all.  In this situation, define a MagicStringTranslator subclass for the Identifier property:
 
    public class IdentifierCode : MagicStringTranslator
    {
        public const string SUBJECT_INFO = "IDENT";
        public const string SUMMARY = "SUMMARY";
        public const string COLLECTION_ITEMS = "COLLT";
        public const string TRADE_ACCOUNT_ACTIVITY = "ACCNT";
        public const string MESSAGE = "MSSGE";
        public const string EMPLOYMENT = "EMPLY";
        public const string INQUIRY_HISTORY = "INQHS";
 
    }
 
Then make the class’ Identifier property of type IdentifierCode:
 
        public IdentifierCode Identifier
            {
                  get{ return new IdentifierCode(segments[(int)Fields.Identifier].Data); }
                  set { } // no-op setter provided so that XmlSerializer can serialize this property
            }
 
Finally, use the Identifier directly, without any need for branching logic:
 
        writer.WriteFullBeginTag("Table style='width: 100%' border='1'");
            writer.WriteFullBeginTag("tr style='background-color:#ccffcc;'");
                writer.WriteFullBeginTag("td style='width: 100%;' colspan='2' align='center'");
                    writer.WriteFullBeginTag("span style='font-size: 9pt;");
                    writer.WriteFullBeginTag("strong");
                    writer.Write(m_record.Identifier.Description);
                    writer.WriteEndTag("strong");
                    writer.WriteEndTag("span");
                writer.WriteEndTag("TD");
            writer.WriteEndTag("TR");
        writer.WriteEndTag("Table");
 
And you're done.
Posted on Friday, March 7, 2008 7:45 AM Coding Practices and Design Patterns | Back to top


Comments on this post: Refactoring to Comply with the DRY principle (Don't Repeat Yourself)

No comments posted yet.
Your comment:
 (will show your gravatar)


Copyright © Chris Falter | Powered by: GeeksWithBlogs.net