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));
        while (contFlag)
                doc = new XPathDocument(ds + "/current");
                navigator = doc.CreateNavigator();
                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);

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

                nodes = navigator.Select("//m:ComponentStream", ns);
                int i = 1;
                    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);
                catch { }
            catch (WebException ex)
                MessageBox.Show("Cannot retrieve stream. Please check Data source.","Demo App");
                xmlParser.AppState = 0;

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.

  • 4
    \$\begingroup\$ I haven't looked at your code thoroughly yet but I can tell you one thing, switching over to using LINQ to XML and the XDocument could cut the code down to a much more reasonable size. \$\endgroup\$ Aug 11 '11 at 4:45
  • 1
    \$\begingroup\$ The code doesn't look complete. What is 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\$ Aug 12 '11 at 1:04
  • \$\begingroup\$ xmlparser is just an inhouse built parser for the specific web service we are using. It will just traverse the navigator and returns a string that we request for based on the parameters provided. Consider that all xmlparser does is returns a string based on the parameters given. \$\endgroup\$
    – swordfish
    Aug 12 '11 at 3:10
  • \$\begingroup\$ Reg linq this is a 2.0 app and linq to xml isnt available in 2.0.(or atleast i cant find one which just works) \$\endgroup\$
    – swordfish
    Aug 12 '11 at 3:11

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):

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
SetFeedRateDouble(); // I think that's what fdrtdbl is? Who knows

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)

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.


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.