3

I wrote a function to convert a hexa string representation (like x00) of some binary data to the data itself.

How to improve this code?

QByteArray restoreData(const QByteArray &data, const QString prepender = "x")
{
    QByteArray restoredData = data;

    return QByteArray::fromHex(restoredData.replace(prepender, ""));
}
Maxim Makhun
  • 2,197
  • 1
  • 22
  • 26
Sylva1n
  • 119
  • 1
  • 7

2 Answers2

1

Your code has a performance problem because of replace(). Replace itself is not very fast, and creating intermediate QByteArray object slows the code down even more. If you are really concerned about performance, you can copy QByteArray::fromHex implementation from Qt sources and modify it for your needs. Luckily, its implementation is quite self-contained. I only changed / 2 to / 3 and added --i line to skip "x" characters.

QByteArray myFromHex(const QByteArray &hexEncoded)
{
    QByteArray res((hexEncoded.size() + 1)/ 3, Qt::Uninitialized);
    uchar *result = (uchar *)res.data() + res.size();

    bool odd_digit = true;
    for (int i = hexEncoded.size() - 1; i >= 0; --i) {
        int ch = hexEncoded.at(i);
        int tmp;
        if (ch >= '0' && ch <= '9')
            tmp = ch - '0';
        else if (ch >= 'a' && ch <= 'f')
            tmp = ch - 'a' + 10;
        else if (ch >= 'A' && ch <= 'F')
            tmp = ch - 'A' + 10;
        else
            continue;
        if (odd_digit) {
            --result;
            *result = tmp;
            odd_digit = false;
        } else {
            *result |= tmp << 4;
            odd_digit = true;
            --i;
        }
    }

    res.remove(0, result - (const uchar *)res.constData());
    return res;
}

Test:

qDebug() << QByteArray::fromHex("54455354"); // => "TEST"
qDebug() << myFromHex("x54x45x53x54"); // => "TEST"

This code can behave unexpectedly when hexEncoded is malformed (.e.g. "x54x45x5" will be converted to "TU"). You can fix this somehow if it's a problem.

Pavel Strakhov
  • 39,123
  • 5
  • 88
  • 127
1

How to improve this code?

Benchmark before optimizing this. Do not do premature optimization.

Beyond the main point: Why would you like to optimize it?

1) If you are really that concerned about performance where this negligible code from performance point of view matters, you would not use Qt in the first place because Qt is inherently slow compared to a well-optimized framework.

2) If you are not that concerned about performance, then you should keep the readability and maintenance in mind as leading principle, in which case your code is fine.

You have not shown any real world example either why exactly you want to optimize. This feels like an academic question without much pratical use to me. It would be interesting to know more about the motivation.

That being said, several improvement items, which are also optimization, could be done in your code, but then again: it is not done for optimization, but more like logical reasons.

1) Prepender is bad name; it is usually called "prefix" in the English language.

2) You wish to use QChar as opposed to QString for a character.

3) Similarly, for the replacement, you wish to use '' rather than the string'ish "" formula.

4) I would pass classes like that with reference as opposed to value semantics even if it is CoW (implicitly shared).

5) I would not even use an argument here for the prefix since it is always the same, so it does not really fit the definition of variable.

6) It is needless to create an interim variable explicitly.

7) Make the function inline.

Therefore, you would be writing something like this:

QByteArray restoreData(QByteArray data)
{
    return QByteArray::fromHex(data.replace('x', ''));
}
László Papp
  • 51,870
  • 39
  • 111
  • 135
  • 1
    You could also opt for asking a `remove(QChar)` method for convenience like in QString. That would even make the code more logical as you are not replacing, but removing in principle. – László Papp May 24 '14 at 10:43