Using QXmlStreamReader to read configuration file of key-value pairs

Question

Background

The following XML snippet contains configuration parameters for our application in key-value pair manner. (Despite using INI files for such purpose may be better, I have to use XML files.)

<configuration>
    <parameter id="serial_port_name">ttyS0</parameter>
    <parameter id="serial_port_baud_rate">9600</parameter>
    <parameter id="check_period">1000</parameter>
    <parameter id="locale">hu_HU</parameter>
    <!-- ... -->
</configuration>

I would like to read the configuration parameters and store them inside a QHash<QString, QString> object with minimal overhead, therefore I've chosen the QXmlStreamReader for iterating through XML elements.

Current code

The toConfigurationMap() method of the following class iterates through the elements of XML file and when a <configuration> element is found, starts another iteration on the level of <parameter> elements to parse them.

class ConfigurationReader {
public:
    explicit ConfigurationReader(QFile& input) : reader(&input) {
    }

    QHash<QString, QString> toConfigurationMap() {
        QHash<QString, QString> output;

        for(auto token = reader.tokenType(); !reader.atEnd();
                token = reader.readNext()) {
            if(token != QXmlStreamReader::StartElement ||
                    reader.name() != "configuration")
                continue;

            while(reader.readNextStartElement()) {
                if(reader.name() == "parameter") {
                    QString id = reader.attributes().value("id").toString();
                    output.insert(id, reader.readElementText());
                }
                else {
                    reader.skipCurrentElement();
                }
            }
        }

        return output;
    }

private:
    QXmlStreamReader reader;
};

Goals

I would like to reduce the complexity of the code above, by somehow take apart the nested loops. In order to the toConfigurationMap() method be more cleaner, I would like to communicate to the reader that "it parses the parameters for each configuration elements" through the code.

Answer 1

Case for not using a class

It's not clear to me whether you need a class for what you are doing. Unless you need to be able to construct an instance of ConfigurationReader and pass it around, I would say get rid of the class. You can have a function with the simple interface

QHash<QString, QString> getConfigurationMap(QFile& input);

I would further suggest putting the function in a namespace that is specific to your app.

namespace MyApp
{
   QHash<QString, QString> getConfigurationMap(QFile& input);
}

Use smaller functions

Instead of one large function, divide the code into multiple functions. Think of what you are doing rather than how you are doing it. The whats usually translate to smaller functions and hows translate to loops, conditionals, branches, etc.

Given that, getConfigurationMap can be implemented as:

namespace MyApp
{
   // Implementation detail.
   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map);

   // User facing interface.
   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The second overload (the one that's listed first) can be implemented as:

namespace MyApp
{
   void skipUntilConfiguration(QXmlStreamReader& reader);

   void readParamters(QXmlStreamReader& reader,
                      QHash<QString, QString>& map);

   // Implementation detail.
   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map);
   {
      skipUntilConfiguration(reader);
      readParamters(reader, map):
   }

   // User facing interface.
   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The code can be wrapped up by implementing the final two helper functions.

namespace MyApp
{
   void skipUntilConfiguration(QXmlStreamReader& reader)
   {
      whiile ( !reader.atEnd() )
      {
         if ( reader.tokenType() == QXmlStreamReader::StartElement &&
              reader.name() == "configuration" )
         {
            return;
         }
      }
   }

   void readParamters(QXmlStreamReader& reader,
                      QHash<QString, QString>& map)
   {
      while ( reader.readNextStartElement() ) {
          if ( reader.name() == "parameter" ) {
              QString id = reader.attributes().value("id").toString();
              map.insert(id, reader.readElementText());
          }
          else {
              reader.skipCurrentElement();
          }
      }
   }

   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map)
   {
      skipUntilConfiguration(reader);
      readParamters(reader, map):
   }

   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The above code assumes that there is only one "configuration" element in your XML file. If that assumption is not correct, the functions need to be tweaked a bit.

Answer 2

Your code looks to be about the expected level of complexity for the task. I wouldn't attempt to reduce it any further - you're unlikely to make it faster, and definitely likely to hinder its clarity.

The nested loop structure accurately reflects the data's structure, and I think there's a lot of value in keeping the two aligned.

The thing I might actually question is the interface exposed by this class. It would be very useful to present a QSettings facade to your configuration file to make it easier to interface with other code. You're not far from being able to do that, but QSettings::registerFormat() wants its reader to have this signature:

bool myReadFunc(QIODevice &device, QSettings::SettingsMap &map);

The input parameter can just as easily be a QIODevice& as a QFile&, given that we only create a reader from it, and it wouldn't be hard to use a QSettings::SettingsMap (which is just an alias of QMap<QString,QVariant>) in place of output.