4
\$\begingroup\$

This is a template for one of our data injection tokens.

When I first looked at the template, it was worse than it is now. I would like some input on this because I think that it can be cleaned up even better.

Side note: this is code that we've received from a vendor to use in conjunction with their application for inserting data into an XML file so that a VBScript can retrieve the data.

Here comes all the fun.

public class CustomFormLoad : ICustomFormLoad
{
    #region ICustomFormLoad Members
    string MachineName = System.Environment.MachineName;

    string conSource;

    public string GetCustomXml(string siteId, int userId, string keyId, string keyXml, string dataXml)
    {
        // connect to the production DB to get the connection source for the machine the token is called from.
        string ConnectSource = "Data Source" ; //removed for security reasons

        SqlConnection connect = new SqlConnection(ConnectSource);
        string SelectStatement = "select ReportServer from TokenServers Where ActiveServerMachine = '" + MachineName + "'";
        SqlCommand myCommand = new SqlCommand(SelectStatement, connect);
        SqlDataReader reader;
        ArrayList myArray = new ArrayList();
        try
        {
            connect.Open();
            reader = myCommand.ExecuteReader();
            while (reader.Read())
            {
                myArray.Add(reader.GetString(0));
            }
            reader.Close();
        }
        catch (Exception e)
        {
            return "<Error desc='Could not determine report server'/>";

        }
        finally
        {
            connect.Close();
        }

        string[] array = myArray.ToArray(typeof(string)) as string[];

        conSource = array[0];

        conSource = conSource.Replace("\\\\", "\\");

        // get case ID from xml document
        XmlDocument xmlDoc = new XmlDocument();
        xmlDoc.LoadXml(dataXml);
        XPathNavigator XpathNav;
        XpathNav = xmlDoc.CreateNavigator();

        XPathExpression expr = XpathNav.Compile("//Record/NoticeData/Case/@CaseID");

        XPathNodeIterator nodes = XpathNav.Select(expr);

        if (nodes.Count == 0)
        {
            return null;
        }
        else
        {
            int CaseID = 0;

            try
            {
                nodes.MoveNext();
                {
                    CaseID = int.Parse(nodes.Current.Value);
                }
            }
            catch
            {
                return "<Error desc='CaseID could not be determined'/>";
            }

            // Create a connection object
            // TODO:  FILL IN THE CONNECTION STRING

            string Connection = "Generic Place Holder"
            SqlConnection con = new SqlConnection(Connection);

            con.Open();

            // INSERT THE SINGLE/MULTIPLE DATA SCRIPT HERE
            //Create a DataAdapter, and then provide the name of the stored procedure.
            // TODO:  FILL IN THE STORED PROCEDURE NAME
            string spName = "spPhoneNums";  //*** IMPORTANT – Add stored procedure name here!
            string xmlElementName = "PhoneNumber";  //*** IMPORTANT – Your data will be injected into an XML file, the name used here will be the name of the node that encapsilates your data!
            string spColName = "";  //*** IMPORTANT – the name of the column the data is located in the stored procedure is listed here for the above xmlElementName!
            string AttName = "PartyType";
            string spAttName = "Code";
            string AttName1 = "phoneHome";  //*** IMPORTANT - the name of the data to be stored as an attribute, should be similar to the name used in the column data is located in stored procedure!
            string spAttName1 = "phoneHome";  //*** IMPORTANT - the name of the column the data is located in the stored procdure the data is listed here for the above attribute!
            string AttName2 = "phoneWork";
            string spAttName2 = "phoneWork";
            string AttName3 = "phoneCell";
            string spAttName3 = "phoneCell";

            //*** For multiple data columns repeat the above AttName and spAttName (e.g. string AttName1 = “AttributeName2”, string spAttName1 = “?” …)

            SqlDataAdapter MyDataAdapter = new SqlDataAdapter(spName, con);

            MyDataAdapter.SelectCommand.CommandType = CommandType.StoredProcedure;

            // Create a DataSet
            DataSet oDS = new DataSet();

            // Add Parameters for the Stored Procedure
            // TODO:  FILL IN THE STORED PROCEDURE PARAMETERS
            MyDataAdapter.SelectCommand.Parameters.Add(new SqlParameter("@CaseID", SqlDbType.Int));  //*** IMPORTANT - The stored procedure used should have a single Parameter named @CaseID!

            MyDataAdapter.SelectCommand.Parameters[0].Value = CaseID;

            // Fill the DataSet with data
            // TODO:  FILL IN THE STORED PROCEDURE NAME
            //MyDataAdapter.Fill(oDS.Tables["tblFileDate"], "dbo.spCaseManagerFileDate");
            MyDataAdapter.Fill(oDS, spName);

            xmlDoc = new XmlDocument();
            XmlNode MainNode = xmlDoc.CreateElement(xmlElementName + "s");
            xmlDoc.AppendChild(MainNode);
            XmlNode Elem;

            foreach (DataRow row in oDS.Tables[0].Rows) //*** loop through each row of returned data and put in xml form
            {
                Elem = xmlDoc.CreateElement(xmlElementName);
                XmlAttribute productAttribute = xmlDoc.CreateAttribute(AttName);
                productAttribute.Value = (string)row[spAttName];
                Elem.Attributes.Append(productAttribute);

                if (!row.IsNull(spAttName1))
                {
                    productAttribute = xmlDoc.CreateAttribute(AttName1);
                    productAttribute.Value = (string)row[spAttName1];
                    Elem.Attributes.Append(productAttribute);
                }

                //*** For multiple data columns repeat the below as necessary:
                if (!row.IsNull(spAttName2))
                {
                    productAttribute = xmlDoc.CreateAttribute(AttName2);
                    productAttribute.Value = (string)row[spAttName2];
                    Elem.Attributes.Append(productAttribute);
                }

                if (!row.IsNull(spAttName3))
                {
                    productAttribute = xmlDoc.CreateAttribute(AttName3);
                    productAttribute.Value = (string)row[spAttName3];
                    Elem.Attributes.Append(productAttribute);
                }
                //string XrefNum = (string)row[spColName];
                //Elem.AppendChild(xmlDoc.CreateTextNode(XrefNum.ToString()));

                MainNode.AppendChild(Elem);
            }

            // Dispose of SQL objects VERY IMPORTANT!!
            MyDataAdapter.Dispose();
            con.Close();
            return xmlDoc.InnerXml;
        }
    }
    #endregion
}

When I first saw this, there were no Using blocks. They had the connections being closed in the wrong order at the bottom, so there was some stuff that wasn't being closed.

I've added quite a bit of code in here.

Edit/Update

It looks like this isn't one of the dll's that I've updated with the Using blocks.

I am, however, working on this specific token, so I am in the position to change the code to use the Using blocks.

\$\endgroup\$
  • \$\begingroup\$ programmers.stackexchange.com/questions/27798/… \$\endgroup\$ – MirroredFate Nov 18 '13 at 22:28
  • \$\begingroup\$ For clarification, that linked question is not about the contents of your code so much as its length. It is simply a general principle as to the length that a function should not need to exceed. Dan Lyon's answer goes over how to actually determine splitting the function. MORE CLARIFICATION: Even if every line of code was well-written, that function is just too long to be easily understood. Split it down to smaller functions that max out at 60 lines, if possible. \$\endgroup\$ – MirroredFate Nov 18 '13 at 22:59
  • \$\begingroup\$ EXAMPLE: For instance, in this specific case, you could have readDataSource, createXMLDoc, and buildXMLFromSource. Or something like that. \$\endgroup\$ – MirroredFate Nov 18 '13 at 23:05
8
\$\begingroup\$

There is a lot here to go over:

Single Responsibility Principle

There are too many things going on in this method. It performs multiple database queries, and it parses/traverses XML. This should be split out into several, narrowly-focused methods.

Database

I would suggest using an ORM of some sort for your database access. There are plenty of solutions out there, some more well-known examples being nHibernate and Microsoft's own Entity Framework. My team has also played around with F# Type Providers.

For the sake of my answer, though, I will assume making a change like that is probably outside the scope of what you are doing. With that in mind, I would suggest using an IDataReader rather than a SqlDataAdapter, since you appear to know the exact structure of the return data. Reading with IDataReader will involve less overhead, and more importantly, it will be easier to read.

XML Building

The older System.Xml objects still have their uses from time to time, but I have found the System.Xml.Linq objects to be far cleaner and easier to work with for most tasks.

A quick example:

     var homeNumbers = new List<string>{"867-5309", "648-8888"};
     var workNumbers = new List<string>{"867-5309", "648-8888"};
     var numbers = new XElement("PhoneNumbers");
     var doc = new XDocument(numbers);
     for(int i = 0; i < homeNumbers.Count; i++)
     {
        var number = new XElement("PhoneNumber");
        numbers.Add(number);
        number.Add(new XAttribute("phoneHome", homeNumbers[i]));
        number.Add(new XAttribute("phoneWork", workNumbers[i]));
     }

Even better would be a way to map data objects from the database code to XML. Normally, this is where I might suggest System.Runtime.Serialization.DataContractSerializer, but it does not support attributes :(

Unused Code

I ran across a number of variables which are not used or assigned and then never read. Some examples include conSource and spColName. I cannot ascertain whether they should be removed or if the code meant to use them has yet to be added.

Constants

Most of the string literals in the code appear to be good candidates for constant values rather than local string variables.

Boxing

There are a few places where unnecessary boxing occurs. Specifically, the ArrayList used to store report servers and the DataSet used to store the second query result will both box values. The former can be easily replaced with a List, which also eliminates the need to perform the ToArray call. The latter is taken care of by changing the database code to use something other than a SqlDataAdapter (see above).

\$\endgroup\$
  • \$\begingroup\$ this code actually takes an XML Document in and adds elements to the document and then returns the document (which is needed by the parent application) so I am not sure that I want to go that way with the Linq, I haven't used it before. so you and @svick have both given me two things that I am going to look into that I haven't used before, both which I think I should already know to begin with List<T> and Linq I have heard a lot about them but have been avoiding having to learn them for lots of reasons. but I want to! \$\endgroup\$ – Malachi Nov 18 '13 at 19:47
  • \$\begingroup\$ BTW, storing strings in an ArrayList is not boxing, because string is already a reference type. Though that doesn't mean it's a good idea to do it. \$\endgroup\$ – svick Nov 18 '13 at 20:22
5
\$\begingroup\$
string MachineName = System.Environment.MachineName;

Why do you have this in a field? When you need to know the machine name, just access the Environment.MachineName property.

string conSource;

I don't see any reason why this is a field. Actually, I don't see any reason why this exists, it's written, but never read.

string ConnectSource

Local variables should be in camelCase, that means connectSource in this case. This applies to other local variables named this way too.

Also, having different variables called conSource and ConnectSource is very confusing. Try to give better names to your variables and don't shorten their names, it will make your code more readable.

string SelectStatement = "select ReportServer from TokenServers Where ActiveServerMachine = '" + Environment.MachineName + "'";

Get into the habit of not forming SQL queries like this. Instead, use parametrized queries. It's cleaner and safer (no chance of SQL injections).

ArrayList myArray = new ArrayList();

Don't ever use ArrayList, it's there just for backwards compatibility with .Net 1.0. Instead use List<T>, in this case, List<string>.

Also, myArray is a very bad name for a variable. Use a descriptive name like servers, or something like that.

One more thing, it looks to me like the contents of this array are never actually used anywhere. Why is all this code even there?

catch (Exception e)
{
    return "<Error desc='Could not determine report server'/>";
}

This is a very bad way to deal with exceptions.

  1. You should catch only specific exceptions that you know about. Quite often, application crash caused by an unhandled exception is the right way to deal with unexpected exceptions, because it means no data is corrupted and you learn quickly about it.

  2. You're throwing away all information about the exception that happened. This will make debugging problems that arise much harder.

  3. I think it should not be the responsibility of this method to format the error response. Instead, it should be centralized in the code that calls this method.

An example of reasonable error handling could look like this:

catch (SqlException e)
{
    throw new FormLoadException("Could not determine report server.", e);
}

I think I'll stop there. On just a few lines, there were quite a lot significant issues. I think that you should first try to improve the code yourself and then you should come back and ask another question about the issues that you missed.

\$\endgroup\$
  • \$\begingroup\$ that exception that is caught, tells us that the table where this information is coming from is wrong. the string that is being returned is inserted into the xml, so when we have an issue with the token it will show in the xml, it is one of the debugging steps. the 3rd party application is a little over complex in what it all does. \$\endgroup\$ – Malachi Nov 18 '13 at 19:33
  • \$\begingroup\$ sorry, conSource is used in my connection string, which I replaced with Generic place holder to protect my connection string. all the input that goes into this comes from a SQL record, and that information is created by the application, so I don't really have to worry about SQL Injection. \$\endgroup\$ – Malachi Nov 18 '13 at 19:38
3
\$\begingroup\$

I have realized that this code doesn't make use of any using statements...

In one spot I have

    string ConnectSource = "Data Source" ; //removed for security reasons

    SqlConnection connect = new SqlConnection(ConnectSource);
    string SelectStatement = "select ReportServer from TokenServers Where ActiveServerMachine = '" + MachineName + "'";
    SqlCommand myCommand = new SqlCommand(SelectStatement, connect);
    SqlDataReader reader;
    ArrayList myArray = new ArrayList();
    try
    {
        connect.Open();
        reader = myCommand.ExecuteReader();
        while (reader.Read())
        {
            myArray.Add(reader.GetString(0));
        }
        reader.Close();
    }
    catch (Exception e)
    {
        return "<Error desc='Could not determine report server'/>";

    }
    finally
    {
        connect.Close();
    }

When I should have something like this:

string SelectStatement = "select ReportServer from TokenServers Where ActiveServerMachine = '" + MachineName + "'"; 
using (SqlConnection connect = new SqlConnection(ConnectSource))
using (SqlCommand myCommand = new SqlCommand(SelectStatement, connect))
{
    ArrayList myArray = new ArrayList();
    SqlDataReader reader;

    try
    {
        connect.Open();
        using (reader = myCommand.ExecuteReader())
        {
            while (reader.Read())
            {
                myArray.Add(reader.GetString(0));
            }
        }
    }
    catch (Exception e)
    {
        return "<Error desc='Could not determine report server'/>";
    }
}

This makes it much cleaner, the only reason that the try/catch is still there is to let the user know that the server couldn't be determined.


There are plenty of opportunities to use using statements/blocks in this code and they should be utilized for safety's sake.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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