Injecting data into XML using a dll

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

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

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{"867-5309", "648-8888"};
     var workNumbers = new List{"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).

StackExchange Code Review Q#35610, answer score: 8