Am I TRWTF?



  • This is a long one. 

    I ran across this recently in a project currently in development.  The purpose of this method is to build a XML document fragment using data retrieved from the database.  Pretty straightforward.  However, I'm of the opinion that this implementation is steaming pile of fail.  I've been going back-and-forth with our senior dev (who is the one who did the code review), trying to convince him that it's a WTF.  This is the same senior dev who has rejected my commits for lack of useless inline comments, as detailed in this Programmers.SE question.

    Am I TRWTF for thinking that this code sucks horribly?  If you were in charge of code reviews, would you let this shite pass?  Bear in mind that we're months away from moving to production and have plenty of time to fix it.

     I changed the variable names and removed XPaths to protect the stupid innocent, but I contend that this crap sucks even with semi-meaningful variable names intact.  The structure, formatting, comments and region tags are all original though.  This is C#, for those who care.

    private void CreatePoliciesXML(ref XmlDocument xml)
            {
                XmlElement element1 = null;
                XmlElement element2 = null;
                DataTable dt = null;

                try
                {
                    #region Get Data
                    XmlNode nNode = xml.SelectSingleNode("...snip XPath...");
                    if (nNode != null && !String.IsNullOrEmpty(nNode.InnerText))
                    {                   
                        using (DataSet ds = GetData(nNode.InnerText))
                        {
                            if (ds != null && ds.Tables.Contains("...snip column name..."))
                            {
                                dt = ds.Tables["...snip column name..."];
                            }
                        }
                    }
                    #endregion

                    //Create Parent Policies Node
                    element1 = CreateNewElement("...snip node name...", string.Empty, xml);

                    #region Create Individual Policy and append to parent
                    if (dt != null && dt.Rows.Count > 0)
                    {
                        for (int i = 0; i < dt.Rows.Count; i++)
                        {
                            #region create a string variable for each column in dataset and two Additional for data from xml
                            string
                                /* Column 01 */  sColumn1 = string.Empty
                                /* Column 02 */, sColumn2 = string.Empty
                                /* Column 03 */, sColumn3 = string.Empty
                                /* Column 04 */, sColumn4 = string.Empty
                                /* Column 05   , sColumn5 Not used */
                                /* Column 06 */, sColumn6 = string.Empty
                                /* Column 07 */, sColumn7 = string.Empty
                                /* Column 08 */, sColumn8 = string.Empty
                                /* Column 09 */, sColumn9 = string.Empty
                                /* Column 10 */, sColumn10 = string.Empty
                                /* Column 11 */, sColumn11 = string.Empty
                                /* Column 12 */, sColumn12 = string.Empty
                                /* Column 13 */, sColumn13 = string.Empty
                                /* Column 14 */, sColumn14 = string.Empty
                                /* Column 15 */, sColumn15 = string.Empty
                                /* Column 16   , sColumn16  Not used */
                                /* Column 17 */, sColumn17 = string.Empty
                                /* Column 18   , sColumn18  Not used */
                                /* Column 19 */, sColumn19 = string.Empty
                                /* Column 20 */, sColumn20 = string.Empty
                                /* Column 21 */, sColumn21 = string.Empty
                                /* Column 22 */, sColumn22 = string.Empty;

                            //create string variable for additional data collected from Customer/Application
                            string sString1 = string.Empty
                                 , sString2 = string.Empty;

                            //Variable to track which column is being read - used in exception handling.
                            string sFailedToRead = string.Empty;
                            #endregion

                            try
                            {                           
                                #region Assign Row values to string

                                /* Column 01 */
                                sFailedToRead = "sColumn1";
                                if (!(dt.Rows[i]["sColumn1"] is DBNull))
                                    sColumn1 = dt.Rows[i]["sColumn1"].ToString().Trim();
                               
                                if (sColumn1.Equals(xml.SelectSingleNode("...snip XPath...").InnerText.Trim()))
                                    continue;

                                /* Column 02 */
                                sFailedToRead = "sColumn2";
                                if (!(dt.Rows[i]["sColumn2"] is DBNull))
                                    sColumn2 = dt.Rows[i]["sColumn2"].ToString().Trim();

                                /* Column 03 */
                                sFailedToRead = "sColumn3";
                                if (!(dt.Rows[i]["sColumn3"] is DBNull))
                                    sColumn3 = dt.Rows[i]["sColumn3"].ToString().Trim();

                                /* Column 04 */
                                sFailedToRead = "sColumn4";
                                if (!(dt.Rows[i]["sColumn4"] is DBNull))
                                    sColumn4 = dt.Rows[i]["sColumn4"].ToString().Trim();

                                /* Column 05     sSSN  Not used */

                                /* Column 06 */
                                sFailedToRead = "sColumn6";
                                if (!(dt.Rows[i]["sColumn6"] is DBNull))
                                    sColumn6 = dt.Rows[i]["sColumn6"].ToString().Trim();

                                /* Column 07 */
                                sFailedToRead = "sColumn7";
                                if (!(dt.Rows[i]["sColumn7"] is DBNull))
                                    sColumn7 = dt.Rows[i]["sColumn7"].ToString().Trim();

                                /* Column 08 */
                                sFailedToRead = "sColumn8";
                                if (!(dt.Rows[i]["sColumn8"] is DBNull))
                                    sColumn8 = RemoveCurrencyFormatting(dt.Rows[i]["sColumn8"].ToString().Trim());

                                /* Column 09 */
                                sFailedToRead = "sColumn9";
                                if (!(dt.Rows[i]["sColumn9"] is DBNull))
                                    sColumn9 = RemoveCurrencyFormatting(dt.Rows[i]["sColumn9"].ToString().Trim());

                                /* Column 10 */
                                sFailedToRead = "sColumn10";
                                if (!(dt.Rows[i]["sColumn10"] is DBNull))
                                    sColumn10 = RemoveCurrencyFormatting(dt.Rows[i]["sColumn10"].ToString().Trim());

                                /* Column 11 */
                                sFailedToRead = "sColumn11";
                                if (!(dt.Rows[i]["sColumn11"] is DBNull))
                                    sColumn11 = RemoveCurrencyFormatting(dt.Rows[i]["sColumn11"].ToString().Trim());

                                /* Column 12 */
                                sFailedToRead = "sColumn12";
                                if (!(dt.Rows[i]["sColumn12"] is DBNull))
                                    sColumn12 = dt.Rows[i]["sColumn12"].ToString().Trim();

                                /* Column 13 */
                                sFailedToRead = "sColumn13";
                                if (!(dt.Rows[i]["sColumn13"] is DBNull))
                                    sColumn13 = dt.Rows[i]["sColumn13"].ToString().Trim();

                                /* Column 14 */
                                sFailedToRead = "sColumn14";
                                if (!(dt.Rows[i]["sColumn14"] is DBNull))
                                    sColumn14 = dt.Rows[i]["sColumn14"].ToString().Trim();

                                /* Column 15 */
                                sFailedToRead = "sColumn15";
                                if (!(dt.Rows[i]["sColumn15"] is DBNull))
                                    sColumn15 = dt.Rows[i]["sColumn15"].ToString().Trim();

                                /* Column 16     sInsuredDOB  Not used */

                                /* Column 17 */
                                sFailedToRead = "sColumn17";
                                if (!(dt.Rows[i]["sColumn17"] is DBNull))
                                    sColumn17 = dt.Rows[i]["sColumn17"].ToString().Trim();

                                /* Column 18     sTerminationDate  Not used */

                                /* Column 19 */
                                sFailedToRead = "sColumn19";
                                if (!(dt.Rows[i]["sColumn19"] is DBNull))
                                    sColumn19 = RemoveCurrencyFormatting(dt.Rows[i]["sColumn19"].ToString().Trim());

                                /* Column 20 */
                                sFailedToRead = "sColumn20";
                                if (!(dt.Rows[i]["sColumn20"] is DBNull))
                                    sColumn20 = dt.Rows[i]["sColumn20"].ToString().Trim();

                                /* Column 21 */
                                sFailedToRead = "sColumn21";
                                if (!(dt.Rows[i]["sColumn21"] is DBNull))
                                    sColumn21 = dt.Rows[i]["sColumn21"].ToString().Trim();

                                /* Column 22 */
                                sFailedToRead = "sColumn22";
                                if (!(dt.Rows[i]["sColumn22"] is DBNull))
                                    sColumn22 = dt.Rows[i]["sColumn22"].ToString().Trim();
                                #endregion
                            }
                            catch (Exception ex)
                            {                           
                                Utilities.PublishExceptionEmail(ex.Message, ex, Utilities.additionalInfo);
                                throw;
                            }

                            try
                            {                           
                                element2 = CreateNewElement("...snip node name...", string.Empty, xml, "id|" + (i + 1000).ToString(), "id|1");

                                #region Create fixed elements for each Policy
                                element2.AppendChild(CreateNewElement("...snip node name...", "No", xml));
                                element2.AppendChild(CreateNewElement("...snip node name...", "Yes", xml));
                                element2.AppendChild(CreateNewElement("...snip node name...", "per", xml));
                                #endregion

                                #region Create Element for each column
                                /* Column 01 */
                                if (!string.IsNullOrEmpty(sColumn1))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn1, xml));

                                /* Column 02 */
                                if (!string.IsNullOrEmpty(sColumn2))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn2, xml));

                                /* Column 03 */
                                if (!string.IsNullOrEmpty(sColumn3))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn3, xml));

                                /* Column 04 */
                                if (!string.IsNullOrEmpty(sColumn4))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn4, xml));
                               
           /* Column 05     sSSN  Not used */

                                /* Column 06 */
                                if (!string.IsNullOrEmpty(sColumn6))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn6, xml));

                                /* Column 07 */
                                if (!string.IsNullOrEmpty(sColumn7))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn7, xml));

                                /* Column 08 */
                                if (!string.IsNullOrEmpty(sColumn8))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn8.StartsWith("LPS") ? "0" : sString1, xml));

                                /* Column 09 */
                                if (!string.IsNullOrEmpty(sColumn9))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn9, xml));

                                /* Column 10 */
                                if (!string.IsNullOrEmpty(sColumn10))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn10, xml));

                                /* Column 11 */
                                if (!string.IsNullOrEmpty(sColumn11))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn11, xml));

                                /* Column 12 */
                                if (!string.IsNullOrEmpty(sColumn12))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn12, xml));

                                /* Column 13 */
                                if (!string.IsNullOrEmpty(sColumn13))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn13, xml));

                                /* Column 14 */
                                if (!string.IsNullOrEmpty(sColumn14))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn14, xml));

                                /* Column 15 */
                                if (!string.IsNullOrEmpty(sColumn15))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn15, xml));

                                /* Column 16  */

                                /* Column 17 */                           
                                if (sPlob == "40" || sPlob == "50" || sPlob == "60" || sPlob == "70")
                                    sColumn17 = "No";

                                if (!string.IsNullOrEmpty(sColumn17))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn17 == "Y" ? "Yes" : "No", xml));

                                /* Column 18  */

                                /* Column 19 */
                                if (!string.IsNullOrEmpty(sColumn19))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn19, xml));

                                /* Column 20 */
                                if (!string.IsNullOrEmpty(sColumn20))
                                {
                                    switch (sColumn20.Trim())
                                    {
                                        case "X":
                                        case "Y": sColumn20 = "Yes"; break;
                                        case "N":
                                        default: sColumn20 = "No"; break;
                                    }
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn21, xml));
                                }

                                /* Column 21 : Used indirectly in Column 17 */

                                /* Column 22 */
                                if (!string.IsNullOrEmpty(sColumn22) && sColumn22.Equals("Y"))
                                    element2.AppendChild(CreateNewElement("...snip node name...", sColumn22, xml));

                                #endregion

                                #region Create element for each user generated values from application

                                nNode = xml.SelectSingleNode("...snip XPath...");
                                if (nNode != null)
                                {
                                    sString1 = nNode.SelectSingleNode("...snip node name...") != null ? nNode.SelectSingleNode("...snip node name...").InnerText : string.Empty;
                                    if (!string.IsNullOrEmpty(sString1))
                                        element2.AppendChild(CreateNewElement("...snip node name...", sString1, xml));

                                    sString2 = nNode.SelectSingleNode("...snip node name...") != null ? nNode.SelectSingleNode("...snip node name...").InnerText : string.Empty;
                                    if (!string.IsNullOrEmpty(sString2))
                                        element2.AppendChild(CreateNewElement("...snip node name...", RemoveCurrencyFormatting(sString2), xml));
                                }
                                #endregion

                                #region Check for existing Nodes in current XML
                                if (!string.IsNullOrEmpty(sColumn1))
                                    AppendExistingData(sColumn1, "Company", ref element2, ref xml);
                                #endregion

                                element1.AppendChild(element2);
                            }
                            catch (Exception ex)
                            {                           
                                Utilities.PublishExceptionEmail(ex.Message, ex, Utilities.additionalInfo);
                                throw;
                            }
                        }
                    }
                    #endregion

                    #region Add manually-entered policies from xml               
                    XmlNodeList nodeList = xml.SelectNodes("...snip XPath...");

                    foreach (XmlNode n in nodeList)
                    {                   
                        element1.AppendChild(n.CloneNode(true));
                    }
                    #endregion

                    #region Append Parent to xml
                    if (element1 != null)
                    {
                        XmlNode node = xml.SelectSingleNode("...snip XPath...");
                        AppendOrReplaceChild(ref node, "...snip node name...", ref element1);
                    }
                    #endregion
                }
                finally
                {
                    if (dt != null)
                        dt.Dispose();
                }
            }


  • 🚽 Regular

    I would tell you who is TRWTF, but unfortunately my answer is in Column 18.



  • No you are not, this code can be certainly condensed to something less hideous

     



  • @RHuckster said:

    I would tell you who is TRWTF, but unfortunately my answer is in Column 18.

    I would have gone with sPlob but that just me. It breaks the pattern for the rest of the columns, I was about to see the hidden figure



  • No, you are not TRWTF, IMHO.  The code is a copy-n-paste mess, and I am of the belief that if you have to copy/paste the same code more then twice, it should be extracted (or refactored, to use the more modern term :) into a separate function/method.  If I were reviewing the code, I would flag that and send it back for changes.

    -=- James.



  • Good to know. It's difficult to tell with the generic variable names, but those 20-something string locals are tightly related and should be rolled into a class. I brought this up and was shot down "because it works".



  • @Smitty said:

    Good to know. It's difficult to tell with the generic variable names, but those 20-something string locals are tightly related and should be rolled into a class. I brought this up and was shot down "because it works".

    Just because something work does not means is shit, I they have the time and disposition they should rework this, we are always time-constrained but that does not mean we should let piss poor code a chance when we can do better.

    That is one of the traits that distinguish a pro from a hack, that drive to do better.  Is a shame there are so few good ones :(



  • @jtwine said:

    No, you are not TRWTF, IMHO.  The code is a copy-n-paste mess, and I am of the belief that if you have to copy/paste the same code more then twice, it should be extracted (or refactored, to use the more modern term :) into a separate function/method.  If I were reviewing the code, I would flag that and send it back for changes.
    Agreed, especially about the copy/paste.  Those column specifications should be refactored into some sort of ColumnSpecification class, objects of which will be added to a ColumnSpecificationCollection.  Or something.

     I would consider this code to be in the pre-design phase.  As in, no design has gone into it yet.  That method needs its own object model.



  • copy/paste the same code more then twice
    Fine if I copy once, paste eighty four, am I right?


  • I would reject and crap all over this review for many many reasons.

    • regions within the body of a method (pet peeve of mine but typically if you need em you're doing something wrong)
    • a method too long to reasonably follow
    • code commenting patterns that make the code harder not easier to follow
    • basic misunderstanding about how to write good code
    • lack of RAII
    • use of a ref parameter, epsecially for a method that returns void

    If one of my senior engineers tried to pass this through a code review I'd seriously question their title.



  • Where to start?  First is taking a node from XML, converting it to a databaset, then back to XML.  That's what XSLT is for.  You could replace the entire thing with an XSLT transform and be done with it.

    If you don't go that route, then there is a lot of repeated code that should be refactored.  Note the repeated use of  an exact 4 lines of code, but with different column names.  Exract those 4 lines to a method, and call them in a loop for each column -- shortens down the code to a few lines.  

     And what is up with sFailedToRead?  It's not used ANYWHERE that I can see.  Get rid of it.

     



  • @Smitty said:

    Good to know. It's difficult to tell with the generic variable names, but those 20-something string locals are tightly related and should be rolled into a class. I brought this up and was shot down "because it works".

    I think we have all seen all manner of programming sin committed in the name of "because it works".

    And no, you are definitely not TRWTF. While this code would probably pass at most places, it is still truly horrific.

    It is mind boggling how little common sense so many of our fellow programmers have. I just had a discussion with one of our guys about how a file he recently committed was probably not named as well as it should have been. The file ran one of 2 database updates, depending on the condition. The file was in the component root and named "Gateway".



  • @nexekho said:

    copy/paste the same code more then twice
    Fine if I copy once, paste eighty four, am I right?
     

    1 / 84 < 2 == true

    your code has passed the review



  • @Smitty said:

                            string sString1 = string.Empty
                                 , sString2 = string.Empty;

    I particularly like how they prefixed 'sString1' and 'sString2' with 's', just in case you couldn't already tell that String1 and String2 are in fact strings.


Log in to reply