HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

XML parsing program in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
programparsingxml

Problem

This is my code which works and compiles but I feel it may be non-optimal, especially the tag mapping and use of the if command.

It reads two .xml files and finds what has been added or deleted on the new file. For new items it uses a map to link a tag name to a category of data (content tag is a description for example). This is important since different files have different naming conventions on the tags.

Finally, it does work on the data depending on its category (coming soon) and prints it.

```
#include "pugi/pugixml.hpp"

#include
#include
#include

int main()
{

std::string var;

const std::map tagMap {
{"id", "id"}, {"description", "content"}, {"url", "web_address"}
};

pugi::xml_document doca, docb;
std::map mapa, mapb;

if (!doca.load_file("a.xml") || !docb.load_file("b.xml")) {
std::cout << "Can't find input files";
return 1;
}

for (auto& node: doca.child("data").children("item")) {
const char* id = node.child_value("id");
mapa[id] = node;
}

for (auto& node: docb.child("data").children("item")) {
const char* idcs = node.child_value("id");
if (!mapa.erase(idcs)) {
mapb[idcs] = node;
}
}

for (auto& eb: mapb) {
// Loop the tagMap, and try and associate the tag name to the content
for (auto& kv : tagMap) {
if (kv.first == "id") {
std::string var = eb.second.child_value(kv.second.c_str());
// Do work on returned value of id tag in the future (I.e validation)
std::cout << var << endl;
}
if (kv.first == "description") {
std::string var = eb.second.child_value(kv.second.c_str());
// Do work on returned value of content tag in the future
std::cout << var << endl;
}
if (kv.first == "url") {
std::string var = eb.second.child_value(kv.second.c_str());
// Do work on

Solution

I will only comment about the following not-fully-implemented part.

for (auto& kv : tagMap) {
     if (kv.first == "id") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of id tag in the future (I.e validation)
     std::cout << var << endl;
     }
     if (kv.first == "description") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of content tag in the future
     std::cout << var << endl;
     }
     if (kv.first == "url") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of web_address tag in the future
     std::cout << var << endl;
     }
 }


Even without much code filling the blanks, it seems that you want to have some associative tag => function data. In which case, you could create an std::unordered_map with strings as keys and associating function pointers to them, generated from captureless lambdas. A small example:

std::unordered_map tasks = {
    { "id", [](const std::string& val) {
        // Do work on returned value of id tag
    }},
    { "description", [](const std::string& val) {
        // Do work on returned value of content tag
    }},
    { "url", [](const std::string& val) {
        // Do work on returned value of web_address tag
    }}
};

for (auto& kv: tagMap) {
    auto it = tasks.find(kv.first);
    if (it != tasks.end()) {
        it->second(eb.second.child_value(kv.second.c_str()));
    }
}


That way, your code is easy to extend to other tags by representing tasks associated to tags as data and storing them into a map. Now, it will only work if you don't need captures, otherwise, you may have to use std::function or some more heavyweight mechanism instead of a big chain of if.

Code Snippets

for (auto& kv : tagMap) {
     if (kv.first == "id") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of id tag in the future (I.e validation)
     std::cout << var << endl;
     }
     if (kv.first == "description") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of content tag in the future
     std::cout << var << endl;
     }
     if (kv.first == "url") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of web_address tag in the future
     std::cout << var << endl;
     }
 }
std::unordered_map<std::string, void(*)(const std::string&)> tasks = {
    { "id", [](const std::string& val) {
        // Do work on returned value of id tag
    }},
    { "description", [](const std::string& val) {
        // Do work on returned value of content tag
    }},
    { "url", [](const std::string& val) {
        // Do work on returned value of web_address tag
    }}
};

for (auto& kv: tagMap) {
    auto it = tasks.find(kv.first);
    if (it != tasks.end()) {
        it->second(eb.second.child_value(kv.second.c_str()));
    }
}

Context

StackExchange Code Review Q#87524, answer score: 4

Revisions (0)

No revisions yet.