Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is the way I'm currently getting data from XML, but it seems to be really inefficient, checking every localname in every iteration. How should I be doing it?

Here is a sample of the XML I am trying to parse, followed by my code.

<?xml version="1.0" encoding="UTF-8"?>
<hash>
  <result>
    <properties type="array">
      <property>
        <last-account-review-at>2011-04-26 00:00:04</last-account-review-at>
        <unit-balance type="integer">-104</unit-balance>
        <daily-consumption type="decimal">10.8</daily-consumption>
        <status>active</status>
        <end-date></end-date>
        <icp-number>0001234567RN602</icp-number>
        <address>
          <property-name nil="true"></property-name>
          <flat-number>3</flat-number>
          <suburb>Suburbia</suburb>
          <street-number>21</street-number>
          <region>Nether</region>
          <street-name>Easy</street-name>
          <district>Metropolis</district>
        </address>
        <start-date>2010-05-05</start-date>
        <status-detail nil="true"></status-detail>
      </property>
    </properties>
    <account-number>9001234567</account-number>
  </result>
  <version>1.0</version>
</hash>

I don't have any issues with the maintainability of my code, it's more that it seems like I shouldn't be comparing against so many things at every iteration.

public static class DataRetrieval
    {

        private static XmlNameTable _nt;
        private static object _propertyElement;
        private static object _unitBalanceElement;
        private static object _dailyConsumptionElement;
        private static object _statusElement;

        static DataRetrieval()
        {
            _nt = new NameTable();
            _propertyElement = _nt.Add("property");
            _unitBalanceElement = _nt.Add("unit-balance");
            _dailyConsumptionElement = _nt.Add("daily-consumption");
            _statusElement = _nt.Add("status");
        }      

        public static bool ParseCustomerData(string raw, out List<PropertyDetails> properties)
        {
            bool parsedOK = false;
            properties = new List<PropertyDetails>();
            PropertyDetails property = null;

            XmlReaderSettings settings = new XmlReaderSettings();
            settings.IgnoreComments = true;
            settings.IgnoreWhitespace = true;
            settings.NameTable = _nt;
            XmlReader reader = XmlReader.Create(new StringReader(raw), settings);

            object localname;

            bool continueReading = true;
            int dataItemsRead = 0;
            const int DataItemsToRead = 3;

            try
            {

                while (continueReading)
                {
                    if (reader.IsStartElement())
                    {
                        localname = reader.LocalName;

                        if (localname == _propertyElement)
                        {
                            if (property != null)
                            {
                                if (dataItemsRead == DataItemsToRead)
                                {
                                    properties.Add(property);
                                    dataItemsRead = 0;
                                }
                                else
                                {
                                    break;
                                }

                            }
                            property = new PropertyDetails();
                            continueReading = reader.Read();

                        }
                        else if (localname == _unitBalanceElement)
                        {
                            property.UnitBalance = reader.ReadElementContentAsInt();
                            dataItemsRead++;
                        }
                        else if (localname == _dailyConsumptionElement)
                        {
                            property.DailyConsumption = reader.ReadElementContentAsDouble();
                            dataItemsRead++;
                        }
                        else if (localname == _statusElement)
                        {
                            if (property.SetStatus(reader.ReadElementContentAsString()))
                            {
                                dataItemsRead++;
                            }
                        }
                        else
                        {
                            continueReading = reader.Read();
                        }
                    }
                    else
                    {
                        continueReading = reader.Read();
                    }
                }

            }
            catch (XmlException e)
            {
                Debug.WriteLine("XmlException: {0}", e.Message);
            }

            parsedOK = dataItemsRead == DataItemsToRead;

            if ((property != null) && (parsedOK))
            {
                properties.Add(property);
            }

            return parsedOK;
        }


    }
share|improve this question
add comment

3 Answers 3

Have a look at

Linq to XML (MSDN)

The venerable Scott Gu has a nice piece of Linq to XML here. That should help get you started!

Using Linq to XML should definately make your code easier to read and maintain.

share|improve this answer
add comment

You could try using XDocument instead.

It would also help to see an example of the xml you're trying to parse in order to properly suggest a refactoring of your code.

EDIT:
You could do this (XDocument + LINQ):

string sXml = @"<?xml version=""1.0"" encoding=""UTF-8""?>
                <hash>
                  <result>
                    <properties type=""array"">
                      <property>
                        <last-account-review-at>2011-04-26 00:00:04</last-account-review-at>
                        <unit-balance type=""integer"">-104</unit-balance>
                        <daily-consumption type=""decimal"">10.8</daily-consumption>
                        <status>active</status>
                        <end-date></end-date>
                        <icp-number>0001234567RN602</icp-number>
                        <address>
                          <property-name nil=""true""></property-name>
                          <flat-number>3</flat-number>
                          <suburb>Suburbia</suburb>
                          <street-number>21</street-number>
                          <region>Nether</region>
                          <street-name>Easy</street-name>
                          <district>Metropolis</district>
                        </address>
                        <start-date>2010-05-05</start-date>
                        <status-detail nil=""true""></status-detail>
                      </property>
                    </properties>
                    <account-number>9001234567</account-number>
                  </result>
                  <version>1.0</version>
                </hash>";

var xml = XDocument.Parse(sXml);

var r = from x in xml.Descendants("property")
        select new 
        {
            unitBalance = x.Element("unit-balance").Value,
            dailyConsumption = x.Element("daily-consumption").Value,
            status = x.Element("status").Value
        };

this._propertyElement = xml.Descendants("property");
this._unitBalanceElement = r.ElementAt(0).unitBalance;
this._dailyConsumptionElement = r.ElementAt(0).dailyConsumption;
this._statusElement = r.ElementAt(0).status;

Note: I didn't include any error checking, just wanted to show some possibilities.

share|improve this answer
    
Personally i love XDocument with linq it provides cleaner and better options. –  Usman Masood Jun 10 '11 at 9:16
add comment

If your'e dealing with a given scheme definition in .xsd you can use the xsd.exe tool to generate matching classes. Using those generated classes provides type safety. Here you can find an easy example.

If you don't like xsd.exe xsd2code is another alternative for code generation.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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