I have an application in which I am querying an XML web service continuously every 2 seconds in a thread. The returned XML is very big and I am retrieving lots of stuff from them using XPath and plugging the values into labels in the GUI, using delegates for thread safety.
But the way I've done this, it looks bizarre to me, and I cannot think out of the box and find a way to optimize the code. It's working perfectly but I think there must be a better way to do this.
public void GetConditions(object activeDs)
{
string ds = (string)activeDs;
XPathDocument doc;
XmlNamespaceManager ns;
XPathNavigator navigator;
XPathNodeIterator nodes;
XPathNavigator node;
Thread unitThread = new Thread(new ParameterizedThreadStart(SetUnits));
unitThread.Start(ds);
while (contFlag)
{
try
{
doc = new XPathDocument(ds + "/current");
navigator = doc.CreateNavigator();
navigator.MoveToFirstChild();
string uri = navigator.LookupNamespace("");
ns = new XmlNamespaceManager(navigator.NameTable);
ns.AddNamespace("m", uri);
string devName = xmlParser.GetXmlValues(ns, "//m:Devm", "name", navigator);
tsa.SetText(devName, currDevName);
string execStatus = xmlParser.GetXmlValues(ns, "//m:Exec", navigator);
SetExecutionStatus(execStatus);
string spndlSpd = xmlParser.GetXmlValues(ns, "//m:Spieed", "subType", "ACTUAL", navigator);
double spndlSpdDbl; double.TryParse(spndlSpd, out spndlSpdDbl);
tsa.SetText(spndlSpdDbl.ToString("0.000"), spndSpdLbl);
string spndlSpdOvr = xmlParser.GetXmlValues(ns, "//m:Spieed", "subType", "OVERRIDE", navigator);
double spndlSpdOvrDbl; double.TryParse(spndlSpdOvr, out spndlSpdOvrDbl);
tsa.SetText(spndlSpdOvrDbl.ToString("0.000"), spndSpOrLbl);
string feedRt = xmlParser.GetXmlValues(ns, "//m:Parate", "subType", "ACTUAL", navigator);
double feedRtDbl; double.TryParse(feedRt, out feedRtDbl);
tsa.SetText(feedRtDbl.ToString("0.000"), feedLbl);
string feedRtOvr = xmlParser.GetXmlValues(ns, "//m:Paedrate", "subType", "OVERRIDE", navigator);
double feedRtOvrDbl; double.TryParse(feedRtOvr, out feedRtOvrDbl);
tsa.SetText(feedRtOvrDbl.ToString("0.000"), fdOrLbl);
string modeTxt = xmlParser.GetXmlValues(ns, "//m:ControllerMode", navigator);
tsa.SetText(modeTxt, modeLbl);
string progTxt = xmlParser.GetXmlValues(ns, "//m:Program", "name", "program", navigator);
tsa.SetText(progTxt, prgNameLbl);
string prtCntFrmStrm = xmlParser.GetXmlValues(ns, "//m:PartCount", navigator);
SetPartCount(prtCntFrmStrm);
BuildAlertList();
nodes = navigator.Select("//m:ComponentStream", ns);
int i = 1;
try
{
while (nodes.MoveNext())
{
node = nodes.Current;
string currComp = node.GetAttribute("component", ns.DefaultNamespace);
string currCompName = node.GetAttribute("name", ns.DefaultNamespace);
if (node.HasChildren)
{
XPathNodeIterator xn = node.SelectChildren(XPathNodeType.Element);
while (xn.MoveNext())
{
if (xn.Current.Name == "Condition")
{
XPathNodeIterator xpni = xn.Current.SelectChildren(XPathNodeType.Element);
while (xpni.MoveNext())
{
XPathNavigator xpn1 = xpni.Current;
string currAlrtType = xpn1.GetAttribute("type", ns.DefaultNamespace);
string currAlrt = xpn1.Name;
string[] addToList = new String[3] { currComp, currAlrtType, currAlrt };
ListViewItem ls = new ListViewItem(addToList);
tsa.AccessControlList(ls, condView);
}
}
}
}
i++;
}
}
catch { }
Thread.Sleep(2000);
}
catch (WebException ex)
{
MessageBox.Show("Cannot retrieve stream. Please check Data source.","Demo App");
xmlParser.AppState = 0;
Console.Write(ex.ToString());
break;
}
}
}
My main constraint is that I need to retrieve all this data and plug it in the GUI. Any pointers would be helpful for me.
3 Answers 3
First and largest thing I see here is the size of this method. When you're looking at a block of code and having difficulty recognizing how to simplify it, think of it like a bunch of data that you want to simplify, first thing you do is look for patterns by breaking the data up into pieces you can quantify. In your case this means breaking out blocks of the code into descriptively named methods.
I would break up all those parse and tsa.SetText things you have into seperate methods (assuming your xmlParser object is a member level variable):
SetCurrentDevName();
SetExecutionStatus(); // this can do the string parsing on it's own from the member level xmlParser
SetSpndlGibberishAmbiguouslyNamedThing(); // spndlSpd is not a descriptive name for anything
SetSpndlSpdOvrDbl();
SetFeedRateDouble(); // I think that's what fdrtdbl is? Who knows
SetFeedRateOverDouble();
SetModeText();
SetProgText();
SetPartCount();
Next, you have a try block without a catch or finally after it, this serves no purpose unless I'm missing something..
Lastly, make your whole nested while block a single seperate method.
Breaking things up like this into seperate methods you will probably begin to see patterns emerge and realize more ways to simplify this.
Edit: and as mentioned earlier, use LINQ and XDocuments, far far simpler than the old xmldocument/xpathnavigator stuff.
One minor change you can do:
new Thread(new ParameterizedThreadStart(SetUnits));
can be written as
new Thread(SetUnits);
On your window or base window class, implement a method like this:
protected virtual void InvokeIfRequired(Action action)
{
if (this.InvokeRequired)
{
this.Invoke(action);
}
else
{
action();
}
}
Then when you want to update a GUI element, do this:
this.InvokeIfRequired(() => textBox.Text = "Some result");
// or a block
this.InvoveIfRequired(() => {
textBoxt2.Text = "Some result 2";
textBoxt3.Text = "Some result 3";
});
You call call InvokeIfRequired()
from your thread, rather than looping on the main thread until there are no exceptions.
Looping on the main thread defeats the purpose of using another thread because execution and therefore window event processing is blocked until the loop completes.
XDocument
could cut the code down to a much more reasonable size. \$\endgroup\$xmlParser
? The methods you call on it doesn't look familiar to me. Is that a third-party parser? An in-house built one? It's hard get an idea of what the documents look like as there's no real pattern to how you use it. That is, coming from an outsider's view. We'll need more detail about what's going on here if you'd want a more complete and well informed answer. \$\endgroup\$