10
\$\begingroup\$

this is a small part of my code that allows me to request a file through a load balancer, reading this XML file gives me the server's name (only node inside the XML Document) if the machine isn't inside the LAN then it won't be able to query the website. I give it a longer timeout to make sure that I don't get a ton of errors emailed to me if the network is laggy.

Here is the XML file Structure.

<?xml version="1.0" encoding="utf-8"?>
<Server>
    JSODYAPP01T
</Server>

Here is the Code that retrieves the information

WebRequest request = WebRequest.Create(xmlLocation);
request.Timeout = 90000;

using (WebResponse response = request.GetResponse())
using (XmlReader xmlReader = XmlReader.Create(response.GetResponseStream()))
{
    while (xmlReader.Read())
    {
        if (xmlReader.NodeType == XmlNodeType.Text)
        {
            serverName = xmlReader.Value.ToString();
            serverName = serverName.Replace("\r", "");
            serverName = serverName.Replace("\n", "");
            serverName = serverName.Replace(" ", "");
        }
    }
}

is there a better way of doing this?

\$\endgroup\$
10
\$\begingroup\$

Linq to XML is your friend

var serverName = XDocument.Load(response.GetResponseStream()).Element("Server").Value;

Your code will be easily broken if someone adds a node to the XML, and your code should be as flexible as possible. Never do such an assumption (It's gonna be the first node)

Your method is blocking, consider making it asynchronous.

using (var response = await request.GetResponseAsync()){

}

And by the way, C# got type inference, so why bother typing the type? Use var instead

var request = WebRequest.Create(xmlLocation);
\$\endgroup\$
  • 1
    \$\begingroup\$ Don't forget .Trim() at the end. \$\endgroup\$ – mjolka Oct 28 '14 at 22:12
  • 1
    \$\begingroup\$ @Malachi Linq is specifically designed to handle loading and manipulating data in a consistent way. The code above allows you to load the XML into a format where you can specifically request the XML element you want. This means that you don't need to worry about formatting issues, XML syntax changes, response changes and whitespace trimming. \$\endgroup\$ – deworde Oct 29 '14 at 8:47
  • \$\begingroup\$ as far as blocking, this is the single task that this application was created for, I don't want it running next to itself, it should run one instance and only one instance at a time, it will return the same results a majority of the time. \$\endgroup\$ – Malachi Oct 29 '14 at 13:55
  • \$\begingroup\$ @Malachi I can't thing of an example where type inference can cause problems, because C# in strongly typed, type inference is not dynamic. Blocking wise, I don't know about your application, but it doesn't sound right to run a process just to get the server name! Does it? I don't know \$\endgroup\$ – Sleiman Jneidi Oct 29 '14 at 14:01
  • \$\begingroup\$ The application connects to a page on a load balanced app server to figure out which app server the user is actually connected to through the Load Balancer. it's a way to create a list of all the people on a specific App Server, or the full load on the system. \$\endgroup\$ – Malachi Oct 29 '14 at 14:02
2
\$\begingroup\$

It should be more efficient to use a Regex to replace all those patterns in one swift move:

serverName = Regex.Replace(xmlReader.Value.ToString(), @"[\r\n ]+", "");

If you'd like to remove any kind of whitespace, it could be even simpler:

serverName = Regex.Replace(xmlReader.Value.ToString(), @"\s+", "");
\$\endgroup\$
  • \$\begingroup\$ REGEX? who uses REGEX? lol JK. for something like that you are probably right, I didn't even stack my Replace statements either, but that sounds more like something a VB programmer would do.. \$\endgroup\$ – Malachi Oct 28 '14 at 21:38

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.