6
\$\begingroup\$

I wrote QJson, a utility class in/for Qt, I need you to take a look at. It is both a JSON parser and serializer, but with extended functionality (going beyond the JSON specification) (see first section of this post).

Features

QJson uses QVariants for parsed values and QStrings for serialized JSON data.

The parser not only understands the JSON language but optionally also this modificatons for "lazy" writers, providing the ability to write the data in a simpler way:

  • Allow unquoted strings (e.g. {foo: bar} instead of {"foo": "bar"}. The end of a string is assumed when either a new-line, non-escaped colon, comma, ] or } is following. The whole string is then trimmed. Strings starting with digits are not allowed. Escaping within the string is allowed, even \ followed by any character is treated like the character itself (if it isn't a special escape sequence like \n). The keywords true, false, null are still parsed as a boolean / null values, not as strings.
  • Allow arrays and objects with whitespaces instead of commas as their separator, also mixed (e.g. {whitespaces only: [1 2 3], commas only: [1,2,3], mixed: [1, 2 3]}.
  • Allow any type for the whole JSON (JSON spec only allows objects and arrays as root values), even empty to simplify error handling.
  • Parse errors are (optionally) reported in a human-readable output (e.g. {foo: bar => "Unexpected end of JSON input.", [foo: bar] => "Unexpected character at index 4").

The serializer has the following features:

  • Format output either compact or nice indented, e.g. compact: [1,2,3,{"foo":"bar"}] vs. indented:

    [
        1,
        2,
        3,
        {
            "foo" : "bar"
        }
    ]
    
  • Treat unknown types either as an error (cancel serialization and return "") or encode them as null.

The interface is similar to PHP's functions json_encode, json_decode:

QString QJson::encode(const QVariant &data, (...) );
QVariant QJson::decode(const QString &json, (...) );

where the optional (...) can contain options to control the parser / serlaizer features from above and out-parameters for error reporting. For details see the code.


Possible improvements

For the following I'm looking for solutions / improvements / reviews:

  • Improving code style: The serializer understands both QVariantList and QStringList, which will be encoded in exactly the same way. So the codes look exactly the same, too. I want to put them in one case, but don't know how.
  • Testing: I don't have enough "evil" input to test my parser (and serializer, but this isn't that problematic, I think). Do you have some?
  • Additional feature: Provide an abstract Serializable interface / helper class for QObjects which can then be serialized and restored using QJson. Either use the properties of the QObject or use manual registration of attributes to the serializer engine, using macros for example: JSON_OBJECT_ATTRIBUTE(foo); => registerJsonObjectAttribute(foo, "foo");. The object is then serialized like {"foo": ...}.

qjson.h:

#ifndef QJSON_H
#define QJSON_H

#include <QString>
#include <QVariant>

class QJson
{
    Q_FLAGS(EncodeOption EncodeOptions)
    Q_FLAGS(DecodeOption DecodeOptions)

public:
    enum EncodeOption
    {
        EncodeUnknownTypesAsNull = 0x01,
        Compact = 0x02
    };
    Q_DECLARE_FLAGS(EncodeOptions, EncodeOption)

    enum DecodeOption
    {
        DecodeObjectsAsHash = 0x01,
        AllowUnquotedStrings = 0x02,
        AllowMissingComma = 0x04,
        AllowLazyJSON = AllowUnquotedStrings | AllowMissingComma
    };
    Q_DECLARE_FLAGS(DecodeOptions, DecodeOption)


    static QString encode(const QVariant &data, QString *errorMessage = 0, int indentation = 4);
    static QString encode(const QVariant &data, EncodeOptions options, QString *errorMessage = 0, int indentation = 4);

    static QVariant decode(const QString &json, QString *errorMessage = 0);
    static QVariant decode(const QString &json, DecodeOptions options, QString *errorMessage = 0);

private:
    QJson();

    static QString encodeData(const QVariant &data, EncodeOptions options, QString *errorMessage, int indentation, QString currentLinePrefix);
    static QString encodeString(QString data);
    static QString encodeByteArray(QByteArray data);

    static QVariant parseValue(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage);
    template<typename ContainerType>
    static QVariant parseObject(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage);
    static QVariant parseArray(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage);
    static QVariant parseString(const QString &json, int &index, bool &success, QString *errorMessage);
    static QVariant parseUnquotedString(const QString &json, int &index, bool &success, QString *errorMessage);
    static QVariant parseNumber(const QString &json, int &index, bool &success, QString *errorMessage);
    static QVariant parseBool(const QString &json, int &index, bool &success, QString *errorMessage);
    static QVariant parseNull(const QString &json, int &index, bool &success, QString *errorMessage);
    static int skipWhitespace(const QString &json, int &index);
    static bool checkAvailable(const QString &json, int &index, bool &success, QString *errorMessage, int minAvailable = 1);
    static bool checkToken(const QString &json, int &index, bool &success, QString *errorMessage, QString token);
};

#endif // QJSON_H

qjson.cpp:

#include "qjson.h"
#include <QStringList>

QJson::QJson()
{
}

/* ----------------------------------------------------------------------------------------------------------------- */
// ENCODER / SERIALIZER
/* ----------------------------------------------------------------------------------------------------------------- */

QString QJson::encode(const QVariant &data, QString *errorMessage, int indentation)
{
    return encodeData(data, EncodeOptions(), errorMessage, indentation, QString());
}

QString QJson::encode(const QVariant &data, EncodeOptions options, QString *errorMessage, int indentation)
{
    return encodeData(data, options, errorMessage, indentation, QString());
}

QString QJson::encodeData(const QVariant &data, EncodeOptions options, QString *errorMessage,
                          int indentation, QString currentLinePrefix)
{
    QString indentedLinePrefix = options.testFlag(Compact) ?
                QString::fromAscii("") :
                (currentLinePrefix + QString::fromAscii(" ").repeated(indentation));

    QString optionalNewLine = options.testFlag(Compact) ?
                QString::fromAscii("") :
                (QString::fromAscii("\n") + currentLinePrefix);

    QString optionalIndentedNewLine = options.testFlag(Compact) ?
                QString::fromAscii("") :
                (QString::fromAscii("\n") + indentedLinePrefix);

    QString encoded;

    switch(data.type())
    {
    case(QVariant::Bool):
        encoded += QString::fromAscii(data.toBool() ? "true" : "false");
        break;

    case(QVariant::Int):
    case(QVariant::UInt):
    case(QVariant::LongLong):
    case(QVariant::ULongLong):
        Q_ASSERT(data.canConvert(QVariant::String));
        encoded = data.toString();
        break;

    case(QVariant::Double):
        encoded = QString::number(data.toDouble(), 'g', 16);
        if(!encoded.contains(QString::fromAscii(".")) && !encoded.contains(QString::fromAscii("e")))
            encoded += ".0";
        break;

    case(QVariant::String):
        encoded = encodeString(data.toString());
        break;

    case(QVariant::ByteArray):
        encoded = encodeByteArray(data.toByteArray());
        break;

    case(QVariant::List):
        {
            encoded = QString::fromAscii("[") + optionalIndentedNewLine;
            QVariantList list = data.toList();
            for(int i = 0; i < list.count(); ++i)
            {
                if(i) encoded += QString::fromAscii(",") + optionalIndentedNewLine;
                encoded += encodeData(list.at(i), options, errorMessage, indentation, indentedLinePrefix);
                if(errorMessage && !errorMessage->isNull())
                    return QString();
            }
            encoded += optionalNewLine + QString::fromAscii("]");
        }
        break;

    case(QVariant::StringList):
        {
            encoded = QString::fromAscii("[") + optionalIndentedNewLine;
            QStringList list = data.toStringList();
            for(int i = 0; i < list.count(); ++i)
            {
                if(i) encoded += QString::fromAscii(",") + optionalIndentedNewLine;
                encoded += encodeData(list.at(i), options, errorMessage, indentation, indentedLinePrefix);
                if(errorMessage && !errorMessage->isNull())
                    return QString();
            }
            encoded += optionalNewLine + QString::fromAscii("]");
        }
        break;

    case(QVariant::Map):
        {
            encoded = QString::fromAscii("{") + optionalIndentedNewLine;
            QVariantMap map = data.toMap();
            QVariantMap::iterator i;
            bool first = true;
            for (i = map.begin(); i != map.end(); ++i)
            {
                if(!first)
                    encoded += QString::fromAscii(",") + optionalIndentedNewLine;
                first = false;
                encoded += encodeString(i.key());
                encoded += options.testFlag(Compact) ? QString::fromAscii(":") : QString::fromAscii(" : ");
                encoded += encodeData(i.value(), options, errorMessage, indentation, indentedLinePrefix);
                if(errorMessage && !errorMessage->isNull())
                    return QString();
            }
            encoded += optionalNewLine + QString::fromAscii("}");
        }
        break;

    case(QVariant::Hash):
        {
            encoded = QString::fromAscii("{") + optionalIndentedNewLine;
            QVariantHash hash = data.toHash();
            QVariantHash::iterator i;
            bool first = true;
            for (i = hash.begin(); i != hash.end(); ++i)
            {
                if(!first)
                    encoded += QString::fromAscii(",") + optionalIndentedNewLine;
                first = false;
                encoded += encodeString(i.key());
                encoded += options.testFlag(Compact) ? QString::fromAscii(":") : QString::fromAscii(" : ");
                encoded += encodeData(i.value(), options, errorMessage, indentation, indentedLinePrefix);
                if(errorMessage && !errorMessage->isNull())
                    return QString();
            }
            encoded += optionalNewLine + QString::fromAscii("}");
        }
        break;

    case(QVariant::Invalid):
        encoded = QString::fromAscii("null");
        break;

    default:
        if(!options.testFlag(EncodeUnknownTypesAsNull))
        {
            if(errorMessage)
                *errorMessage = QString::fromAscii("Can't encode this type of data to JSON: %1")
                    .arg(data.typeName());
            return QString();
        }
        encoded = QString::fromAscii("null");
        break;
    }

    return encoded;
}

QString QJson::encodeString(QString data)
{
    QString encoded;
    encoded.append(QChar::fromAscii('"'));
    for(int i = 0; i < data.length(); ++i)
    {
        QChar ch = data.at(i);

        // printable ASCII character?
        if(ch.unicode() >= 32 && ch.unicode() < 128)
            encoded.append(ch);
        else
        {
            switch(ch.unicode())
            {
            case(8):
                encoded.append(QString::fromAscii("\\b"));
                break;
            case(9):
                encoded.append(QString::fromAscii("\\t"));
                break;
            case(10):
                encoded.append(QString::fromAscii("\\n"));
                break;
            case(12):
                encoded.append(QString::fromAscii("\\f"));
                break;
            case(13):
                encoded.append(QString::fromAscii("\\r"));
                break;
            case('"'):
                encoded.append(QString::fromAscii("\\\""));
                break;
            case('\\'):
                encoded.append(QString::fromAscii("\\\\"));
                break;
            case('/'):
                encoded.append(QString::fromAscii("\\/"));
                break;
            default:
                encoded.append(QString::fromAscii("\\u") + QString::number(ch.unicode(), 16)
                               .rightJustified(4, QChar::fromAscii('0')));
            }
        }
    }
    encoded.append(QChar::fromAscii('"'));
    return encoded;
}

QString QJson::encodeByteArray(QByteArray data)
{
    return encodeString(QString::fromLocal8Bit(data));
}


/* ----------------------------------------------------------------------------------------------------------------- */
// DECODER / PARSER
/* ----------------------------------------------------------------------------------------------------------------- */

QVariant QJson::decode(const QString &json, QString *errorMessage)
{
    return decode(json, DecodeOptions(), errorMessage);
}

QVariant QJson::decode(const QString &json, DecodeOptions options, QString *errorMessage)
{
    //there are currently no options defined
    Q_UNUSED(options);

    bool success = true;
    if(!json.isNull() && !json.isEmpty()) {
        int index = 0;
        return parseValue(json, index, options, success, errorMessage);
    }
    else
        // To simplify things, this is not treated as an error but as valid input.
        return QVariant();
}

QVariant QJson::parseValue(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage)
{
    skipWhitespace(json, index);
    if(!checkAvailable(json, index, success, errorMessage))
    {
        success = false;
        if(errorMessage)
            *errorMessage = QString("Expected more input at position %1").arg(index);
        return QVariant();
    }

    switch(json[index].toAscii())
    {
    case '"':
        return QJson::parseString(json, index, success, errorMessage);

    case '{':
        if(options & DecodeObjectsAsHash)
            return QJson::parseObject<QVariantHash>(json, index, options, success, errorMessage);
        else
            return QJson::parseObject<QVariantMap>(json, index, options, success, errorMessage);

    case '[':
        return QJson::parseArray(json, index, options, success, errorMessage);

    case 't':
    case 'f':
        if(options & AllowUnquotedStrings)
            return QJson::parseUnquotedString(json, index, success, errorMessage);
        else
            return QJson::parseBool(json, index, success, errorMessage);

    case 'n':
        if(options & AllowUnquotedStrings)
            return QJson::parseUnquotedString(json, index, success, errorMessage);
        else
            return QJson::parseNull(json, index, success, errorMessage);

    case '0':
    case '1':
    case '2':
    case '3':
    case '4':
    case '5':
    case '6':
    case '7':
    case '8':
    case '9':
    case '-':
        return QJson::parseNumber(json, index, success, errorMessage);

    default:
        if(options & AllowUnquotedStrings)
            return QJson::parseUnquotedString(json, index, success, errorMessage);
        else
        {
            success = false;
            if(errorMessage)
                *errorMessage = QString("Unexpected character at position %1").arg(index);
            return QVariant();
        }
    }
}

template<typename ContainerType>
QVariant QJson::parseObject(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage)
{
    Q_ASSERT(json[index] == '{');
    index++;
    skipWhitespace(json, index);

    ContainerType object;

    while(checkAvailable(json, index, success, errorMessage))
    {
        if(json[index] == QChar::fromAscii('}'))
        {
            index++;
            return object;
        }
        else
        {
            QString key = QJson::parseValue(json, index, options, success, errorMessage).toString();
            if(!success)
                return QVariant();

            skipWhitespace(json, index);
            checkAvailable(json, index, success, errorMessage);

            if(json[index] == QChar::fromAscii(':'))
                index++;
            else
            {
                success = false;
                if(errorMessage)
                    *errorMessage = QString("Expected colon at position %1").arg(index);
                return QVariant();
            }

            skipWhitespace(json, index);

            QVariant value = QJson::parseValue(json, index, options, success, errorMessage);
            if(!success)
                return QVariant();

            // Add the key / value pair to the object
            object.insert(key, value);

            int skippedWhitespaces = skipWhitespace(json, index);
            checkAvailable(json, index, success, errorMessage);

            switch(json[index].toAscii())
            {
            case ',':
                index++;
                skipWhitespace(json, index);
                break;

            case '}':
                //'}' will be processed in the next iteration
                break;

            default:
                // Only allow missing comma if there is at least one whitespace instead of the comma!
                if((options & AllowMissingComma) && skippedWhitespaces > 0)
                    break;
                else
                {
                    success = false;
                    if(errorMessage)
                        *errorMessage = QString("Unexpected character at index %1").arg(index);
                    return QVariant();
                }
            }
        }
    }

    return QVariant();
}

QVariant QJson::parseArray(const QString &json, int &index, DecodeOptions options, bool &success, QString *errorMessage)
{
    Q_ASSERT(json[index] == '[');
    index++;
    skipWhitespace(json, index);

    QVariantList array;

    while(checkAvailable(json, index, success, errorMessage))
    {
        if(json[index] == QChar::fromAscii(']'))
        {
            index++;
            return array;
        }
        else
        {
            QVariant value = QJson::parseValue(json, index, options, success, errorMessage);
            if(!success)
                return QVariant();

            // Add the value pair to the array
            array.append(value);

            int skippedWhitespaces = skipWhitespace(json, index);
            checkAvailable(json, index, success, errorMessage);

            switch(json[index].toAscii())
            {
            case ',':
                index++;
                skipWhitespace(json, index);
                break;

            case ']':
                //']' will be processed in the next iteration
                break;

            default:
                // Only allow missing comma if there is at least one whitespace instead of the comma!
                if((options & AllowMissingComma) && skippedWhitespaces > 0)
                    break;
                else
                {
                    success = false;
                    if(errorMessage)
                        *errorMessage = QString("Unexpected character at index %1").arg(index);
                    return QVariant();
                }
            }
        }
    }

    return QVariant();
}

QVariant QJson::parseString(const QString &json, int &index, bool &success, QString *errorMessage)
{
    Q_ASSERT(json[index] == '"');
    index++;

    QString string;
    QChar ch;

    while(checkAvailable(json, index, success, errorMessage))
    {
        ch = json[index++];

        switch(ch.toAscii())
        {
        case '\\':
            // Escaped character
            if(!checkAvailable(json, index, success, errorMessage))
                return QVariant();
            ch = json[index++];
            switch(ch.toAscii())
            {
            case 'b':
                string.append('\b');
                break;
            case 'f':
                string.append('\f');
                break;
            case 'n':
                string.append('\n');
                break;
            case 'r':
                string.append('\r');
                break;
            case 't':
                string.append('\t');
                break;
            case 'u':
                if(!checkAvailable(json, index, success, errorMessage, 4))
                    return QVariant();
                string.append(QChar(json.mid(index, 4).toInt(0, 16)));
                index += 4;
                break;
            default:
                string.append(ch);
            }
            break;

        case '"':
            // End of string
            return QVariant(string);

        default:
            string.append(ch);
        }
    }

    return QVariant();
}

QVariant QJson::parseUnquotedString(const QString &json, int &index, bool &success, QString *errorMessage)
{
    QString string;
    QChar ch;

    bool end = false;
    while(!end && checkAvailable(json, index, success, errorMessage))
    {
        ch = json[index++];

        switch(ch.toAscii())
        {
        case '\\':
            // Escaped character
            if(!checkAvailable(json, index, success, errorMessage))
                return QVariant();
            ch = json[index++];
            switch(ch.toAscii())
            {
            case 'b':
                string.append('\b');
                break;
            case 'f':
                string.append('\f');
                break;
            case 'n':
                string.append('\n');
                break;
            case 'r':
                string.append('\r');
                break;
            case 't':
                string.append('\t');
                break;
            case 'u':
                if(!checkAvailable(json, index, success, errorMessage, 4))
                    return QVariant();
                string.append(QChar(json.mid(index, 4).toInt(0, 16)));
                index += 4;
                break;
            default:
                string.append(ch);
            }
            break;

        case ':':
        case ',':
        case ']':
        case '}':
        case '\n':
            // End of string (was one character before this!)
            end = true;
            index--;
            break;

        default:
            string.append(ch);
        }
    }

    //trim string
    string = string.trimmed();

    //handle keywords
    if(string == "true")
        return QVariant(true);
    if(string == "false")
        return QVariant(false);
    if(string == "null")
        return QVariant();

    return QVariant(string);
}

QVariant QJson::parseNumber(const QString &json, int &index, bool &success, QString *errorMessage)
{
    int end = index;
    bool endFound = false;
    while(!endFound && end < json.length())
    {
        switch(json[end].toAscii())
        {
        case '0':
        case '1':
        case '2':
        case '3':
        case '4':
        case '5':
        case '6':
        case '7':
        case '8':
        case '9':
        case '.':
        case 'e':
        case 'E':
        case '+':
        case '-':
            end++;
            break;

        default:
            endFound = true;
            break;
        }
    }

    QString numberStr = json.mid(index, end - index);
    index = end;

    QVariant result;
    bool ok;

    // is floating point number?
    if(numberStr.contains(QChar::fromAscii('.')) ||
       numberStr.contains(QChar::fromAscii('e')) ||
       numberStr.contains(QChar::fromAscii('E')))
        result = QVariant(numberStr.toDouble(&ok));
    else
        result = QVariant(numberStr.toLongLong(&ok));

    if(ok)
        return result;
    else {
        success = false;
        if(errorMessage)
            *errorMessage = QString("Can't parse number at position %1: %2").arg(index).arg(numberStr);
        return QVariant();
    }
}

int QJson::skipWhitespace(const QString &json, int &index)
{
    int skipped = 0;
    while(index < json.size())
    {
        switch(json[index].toAscii())
        {
        case ' ':
        case '\t':
        case '\n':
        case '\r':
            index++;
            skipped++;
            break;
        default:
            return skipped;
        }
    }
    return skipped;
}

QVariant QJson::parseBool(const QString &json, int &index, bool &success, QString *errorMessage)
{
    if(checkToken(json, index, success, errorMessage, "true"))
    {
        index += 4;
        return QVariant(true);
    }
    else if(checkToken(json, index, success, errorMessage, "false"))
    {
        index += 5;
        return QVariant(false);
    }
    else
    {
        success = false;
        if(errorMessage)
            *errorMessage = QString("Can't parse value at position %1").arg(index);
        return QVariant();
    }
}

QVariant QJson::parseNull(const QString &json, int &index, bool &success, QString *errorMessage)
{
    if(checkToken(json, index, success, errorMessage, "null"))
    {
        index += 4;
        return QVariant(true);
    }
    else
    {
        success = false;
        if(errorMessage)
            *errorMessage = QString("Can't parse value at position %1").arg(index);
        return QVariant();
    }
}

bool QJson::checkAvailable(const QString &json, int &index, bool &success, QString *errorMessage, int minAvailable)
{
    if(index + minAvailable > json.length())
    {
        success = false;
        if(errorMessage)
            *errorMessage = QString("Unexpected end of JSON input.");
        return false;
    }
    else
        return true;
}

bool QJson::checkToken(const QString &json, int &index, bool &success, QString *errorMessage, QString token)
{
    return checkAvailable(json, index, success, errorMessage, token.length())
            && json.mid(index, token.length()) == token;
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

In general

I like that you didn't try and force OO on something that works very well without it, but it would be simpler, cleaner if you were to just put all those functions in a namespace instead of a class (IIRC the metadata-generating macros for enums work outside classes, too). you can use global static functions or anonymous namespaces in your .cpp file for the private stuff. And also, it is very nice that you don't try to handle I/O and some other stuff that library writers are so keen on doing. SRP all the way, it's great :)

Error handling

There is one thing I don't particularly like about your code:

QString *errorMessage

I can tell by your coding style that you try to follow the Qt coding guidelines, which is fantastic for a Qt library, but this kind of error handling is not particularly useful. You should stick to the qWarning/qDebug etc. kind of error reporting, since Q_ASSERT does that anyway (they can be redirected from external code, without modifying your library if your user wants to). Or even: use exceptions - Qt is huge, so avoiding exceptions does result in a great size reduction for them, but your little library would not suffer that much. The problem with the current approach is that it limits the caller to one way of handling the error: presenting the (by the way not localized) string to the user.

Possible improvements

There is some unneeded duplication, for example parseString and parseUnquotedString looks quite similar, and I noticed other parts, too.

Consistency in style: it's pretty good, and again, props for sticking to the Qt guidleines. One thing I noticed though, is that you use parentheses around case labels (which is alright, if somewhat alien to me personally), but you forget it in some places.

Other

The serializer understands both QVariantList and QStringList, which will be encoded in exactly the same way. So the codes look exactly the same, too. I want to put them in one case, but don't know how.

Simple (if I understood your intention correctly):

case(QVariant::List):
case(QVariant::StringList):
        {
            encoded = QString::fromAscii("[") + optionalIndentedNewLine;
            QStringList list = data.toStringList();
            for(int i = 0; i < list.count(); ++i)
            {
                if(i) encoded += QString::fromAscii(",") + optionalIndentedNewLine;
                encoded += encodeData(list.at(i), options, errorMessage, indentation, indentedLinePrefix);
                if(errorMessage && !errorMessage->isNull())
                    return QString();
            }
            encoded += optionalNewLine + QString::fromAscii("]");
        }
        break;

Additional features

I think the QObject-serialization is already possible with QScriptEngine (although you can probably implement it smaller). One thing I would love to see is an implementation of json-schema. It is still a draft, but there are already some implementations out there (you can google them). It adds validation capability to JSON (probably not as powerful as XML schemas and DTD, but it's something).

\$\endgroup\$
7
  • \$\begingroup\$ Error handling: Q_ASSERT is for internal asserts (things my code really asserts != input should be like). I tried to follow Qt's way to handle conversion errors; see: QString::toInt(int base, bool *ok = 0). What do you mean by "not localized string"? That I don't present where the error occured? --- Parentheses around cases: I do so when I define new variables (compiler warning if I omitted {}). --- Code for QStringList + QVariantList: your version is wrong, since QStringLists contain strings (encodeData(QString)) and QVariantList contain variants (encodeData(QVariant) in case line 6). \$\endgroup\$
    – leemes
    Commented May 20, 2012 at 19:49
  • \$\begingroup\$ (continuation of previous comment) --- Code duplication parseString + parseUnquotedString: The problem is, that some parts of the code differ from each other, for example the exit condition and escaping (in JSON strings, one can't escape any character, e.g. \:, which I allow in unquoted strings since : would terminate the string). --- Namespace vs. utility class: Thanks, I'm not very familiar with namespaces; I use them very rarely. So I just didn't see that it's a nice alternative to a utility class. --- Json-scheme: maybe :) --- Summarizing, thank you for your review & comments! \$\endgroup\$
    – leemes
    Commented May 20, 2012 at 19:54
  • \$\begingroup\$ By "not localized" I mean that if I were to use your library in an application that supports multiple languages I would have a hard time using the messages provided this way. Parentheses: I did mean parentheses, not curly braces. See case('"'): vs. case '"':. To solve the code duplication, you should extract the common parts and call it from both, then deal with the differences in each one. \$\endgroup\$ Commented May 20, 2012 at 20:21
  • \$\begingroup\$ Also, I'm familiar with that way of error handling all around Qt, but it does not work well with strings. It would be better if you could use an enum for error codes in a similar fashion. Regarding assert, I was just pointing out that it will report the error in qDebug(), so it would not be the same as your other way of reporting errors. \$\endgroup\$ Commented May 20, 2012 at 20:25
  • \$\begingroup\$ Thanks for the hint with enum errors. That should solve the localisation problem. Misunderstanding with the parenthesis thing in case labels; I will change that. I only use asserts where I am sure that the assersion has to be true, even for wrong inputs. I never use assertions to check user input. That's what I tried to make clear by saying "things my code really asserts != input should be like". Regarding code duplicates: The problem is that the code parts look like the same (same code) but are different (different types / overloads of the same method). I maybe could use macros... \$\endgroup\$
    – leemes
    Commented May 20, 2012 at 20:32
2
\$\begingroup\$

Extensions to the standard.

I know you mean well with the additional features.

  • Allow unquoted strings (e.g. {foo: bar} instead of {"foo": "bar"}.
  • Allow arrays and objects with whitespaces instead of commas as their separator
  • Allow any type for the whole JSON

But these to me are just bugs. Stick to the standard otherwise your code will not spot bugs that the standard was designed to explicitly prevent.

We had similar home grown json parser at work (that we had to eventually throw out) because it led to so many other problems along the way. Extending the standard (because you think it is not required as a bad way to go in coding).

Design:

You are converting from a QString into a JSON DOM (and vice versa). In C++ I would have expected you to convert to from a stream.

I find the use of QString quite limiting as I must convert all sources into a string first before use. This feels very error prone as I must convert the incoming stream to UTF-16 (the type held by the QChar character). Not a horrendous task but one that you must remember to do (designing software is about designing it so that it can not be used incorrectly).

Character encoding

QChar is UTF-16. But in a lot of places in your code you are converting to ascii() before appending to the QString. This is distracting and seems like an unnecessary round trip. Just add UTF-16 characters to the string.

\$\endgroup\$
7
  • \$\begingroup\$ The extensions are optional. You need to explicitly enable them when using QJson. When you use QJson::parse without any arguments but the json string, it behaves (at least it should) with respect to the specification. Thanks for the feedback about character encoding. Maybe I add an interface for binary <-> JSON conversion and use binary data internally. \$\endgroup\$
    – leemes
    Commented May 21, 2012 at 20:20
  • \$\begingroup\$ Note: The JSON RFC explicitly states that all JSON is from Unicode files (no other formats are supported). So there should be no need to transcode. But you do need to support UTF-8/UTF-16/UTF-32 encoding. \$\endgroup\$ Commented May 21, 2012 at 20:22
  • \$\begingroup\$ When I translate my code to use binary data internally instead of QStrings, should I assume that the input is UTF-8 encoded? And another question about encoding: Is it possible to encode real-binary data in JSON? I currently use encodeString(QString::fromLocal8Bit(data)) to encode QByteArrays. If data hasn't to be UTF-8 encoded binary data, I can't do QString::fromUtf8(data) or treat it like an already encoded string. I hope you understand, I'm a bit confused about my own question right now :D \$\endgroup\$
    – leemes
    Commented May 21, 2012 at 20:29
  • \$\begingroup\$ See my last comment. \$\endgroup\$ Commented May 21, 2012 at 20:41
  • \$\begingroup\$ "But you do need to support UTF-8/UTF-16/UTF-32 encoding" => Do I really need to detect this by my own? I guess QString is far more straightforward in this circumstance. \$\endgroup\$
    – leemes
    Commented May 21, 2012 at 20:53

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.