2
\$\begingroup\$

I am working with an API that delivers some data in a QStringList or basically a list of strings. The data contains keys and values but it is not a map or anything like that...just plain list.

The list has always an even number of elements where each pair are like key, value.

I need to efficiently create an object out of this list. I came up with a function that checks existence of a given key and returns it's associated value as a QVariant which then can easily convert to a given type. Here is my code:

#include <QCoreApplication>
#include <QVariant>
#include <QDebug>


struct MyData {
    MyData() = default;

    QString myString;
    bool    myBool;
    int     myInt;
    float   myFloat;
};

QVariant getValueByKey(const QStringList& list, const QString& key) {
    auto keyIndex = list.indexOf(key);
    qDebug() << "index of" << key << "is" << keyIndex;
    if(keyIndex != -1 && list.size() > (keyIndex + 1)) {
        return QVariant(list.at(keyIndex + 1));
    }

    return QVariant();
}

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);    

    QStringList list {     // Example list 
        "KEY-STRING",   "String",
        "KEY-BOOL",     "true",
        "KEY-INT",      "2",
        "KEY-FLOAT",    "3.14"
    };

    MyData data;
    
    data.myString = getValueByKey(list, "KEY-STRING").toString();
    data.myBool = getValueByKey(list, "KEY-BOOL").toBool();
    data.myInt = getValueByKey(list, "KEY-INT").toInt();
    data.myFloat = getValueByKey(list, "KEY-FLOAT").toFloat();

    qDebug() << "The string value is:\t" << data.myString.remove("\"");
    qDebug() << "The bool value is:\t" << data.myBool;
    qDebug() << "The int value is:\t" << data.myInt;
    qDebug() << "The float value is:\t" << data.myFloat;
    
    // Test arithmetic
    qDebug() << "myInt * myFloat:\t" << data.myInt * data.myFloat;

    qDebug() << "Testing invalid key: "
             << getValueByKey(list, "damn-son").toString();

    return a.exec();
}

My Questions are:

  • Does this code make sense?
  • Is there a better way to do such task?
  • My getValue function returns an empty QVariant, which is the same as an empty string. Is there a way to tell the caller of the function that something had gone wrong? is a pair of bool,qvariant senseful?
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

your questions are mostly concerned with what getValueByKey does. If I understand the Qt function names correctly it looks sane. You already saw that there is a problem with the function, ie. it will return a valid value in case of an error.

I suggest converting the complete list to a std::map<std::string, std::string> and handling errors there. Once you have that map you can easily use STL functionality to get your values and use the usual checks to find out if a value exists.

You are converting QString to QVariant in your loop. Why not just hand out the string and let the caller decide what to do with it?

Once your function is finished, you should remove the debug statement.

Also: from your question it sounds as if the API is fixed and cannot be changed. However, returning a list with even length to avoid building a map is a really bad design decision and should be changed. Maybe you have a chance of changing this.

\$\endgroup\$
2
  • \$\begingroup\$ Ideally you want to avoid having either a list or a map, and just have the API return a struct that contains all the information. But unfortunately you sometimes have no control over the libraries you have to use. \$\endgroup\$
    – G. Sliepen
    Commented Jul 22, 2022 at 6:18
  • \$\begingroup\$ Well sadly I can not change the API, because it actually comes from a DBUS message where under the hood is just array of strings. \$\endgroup\$
    – DEKKER
    Commented Jul 22, 2022 at 10:12
1
\$\begingroup\$

Are the keys always in the same order?

You mention an API that delivers data as a QStringList. Do you know if the keys are always in the same order in that list? If so, you don't need to scan the list to find a matching key, you can just use its index in the list. Note that you should only rely on this if the API guarantees the order, or if you have some control over the code that produces the list and can ensure the order.

Don't return a variant

If the list would store values already in a variant, then it would be fine to return that varant in getValueByKey(). But since the list only contains strings, converting to and from a variant adds unnecessary overhead. Consider making functions that return the desired type immediately: getStringByKey(), getIntByKey(), and so on. It could be implemented like so:

const QString& getStringByKey(const QStringList& list, const QString& key) {
    if (auto index = list.indexOf(key); index != -1) {
        return list[index];
    } else {
        // Handle error here
    }
}

const int getIntByKey(const QStringList& list, const QString& key) {
    return getStringByKey(list, key).toInt();
}

Handling non-existing keys

There are several ways to handle it. If it is exceptional that a key does not exist (for example, because the API guarantees it should), consider throwing an exception. This way you don't have to deal with returning a value that indicates the key doesn't exist.

Otherwise, consider using std::optional<> for the return value.

\$\endgroup\$
1
\$\begingroup\$

Does this code make sense?

The code looks time inefficient due to the frequent indexOf calls for strings, but it makes sense because it solves the problem.

Is there a better way to do such task?

From the problem statement, I assume that keys consist of ASCII characters, and a kind of deserialization function is needed. Here, possible solution based on this answer (requires c++14).

// file data.h
#ifndef DATA_H
#define DATA_H

#include <QString>

struct Data
{
    QString mString;
    bool mBool;
    int mInt;
    float mFloat;
};

Data deserialize(const QStringList& plainData);

#endif

// file data.cpp
#include "data.h"

#include <QVariant>

namespace
{

constexpr size_t latin1Code(char ch)
{
    return static_cast<size_t>(ch);
}

constexpr size_t latin1Code(QChar ch)
{
    return static_cast<size_t>(ch.toLatin1());
}

template<typename T>
constexpr size_t hashCString(const T* str)
{
    size_t strHash = 0u;
    for (const T* p = str; *p != '\0'; ++p)
        strHash = strHash * 33 + latin1Code(*p);

    return strHash;
}

size_t hashString(const QString& str)
{
    return hashCString(str.data());
}

constexpr size_t hashString(const char* str)
{
    return hashCString(str);
}

} // namespace    

Data deserialize(const QStringList& plainData)
{
    Data d{};
    for (int i = 0; i + 1 < plainData.size(); i += 2) {
        const QString &key = plainData.at(i);
        const QString &value = plainData.at(i + 1);

        switch (hashString(key)) {
        case hashString("KEY-STRING"):
            d.mString = value;
            break;

        case hashString("KEY-BOOL"):
            d.mBool = QVariant(value).toBool();
            break;

        case hashString("KEY-INT"):
            d.mInt = value.toInt();
            break;

        case hashString("KEY-FLOAT"):
            d.mFloat = value.toFloat();
            break;

        default:
            break;
        }
    }

    return d;
}

I suppose, this solution is more time efficient because it compares integers instead of strings, although it computes a hash value of every second list element. To reason about performance improvements, one should test both solutions with actual data.

Note that the hashString overload with const char * parameter must be constexpr to appear after the case. The c++14 requirement is due to the hashCString implementation given merely as an example. It has the for loop, and it will likely not compile with c++11.

Is there a way to tell the caller of the function that something had gone wrong?

This problem has no universal solution. An actual solution depends on particular safety requirements. In general, I would recommend checking error handling guidelines.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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