More efficient way to retrieve data from JSON

Question

I am using the jsmn JSON parser (source code) to get extract data from a JSON string. jsmn stores the data in tokens that just point to the token boundaries in the JSON string instead of copying the data. For example, jsmn will create tokens like:

• Object [0..31]

• String [3..7], String [12..16], String [20..23]

• Number [27..29]

This method is used to retrieve the actual characters between those values (for String objects):

char* getTextFromJSON(const char *json)
{
    json_parser p;
    jsontok_t tokens[15];
    jsontok_t key;

    if (!json) return NULL;

    initJsonParser(&p);
    parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
    for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)
    {
        key = tokens[i];
        unsigned int length = key.end - key.start;
        char keyString[length + 1];
        memcpy(keyString, &json[key.start], length);
        keyString[length] = '\0';
        if(!strcmp(keyString, "utterance"))
        {
            key = tokens[i + 1];
            length = key.end - key.start;
            char* keyString = (char*) malloc(length + 1);
            memcpy(keyString, &json[key.start], length);
            keyString[length] = '\0';
            return keyString;
        }
    }
    return NULL;
}

Here are some JSON examples that would be thrown into the parser:

{"status":0,"id":"432eac38858968c108899cc6c3a4bade-1","hypotheses" [{"utterance":"test","confidence":0.84134156}]}

{"status":5,"id":"695118aaa3d01dc2ac4aa8054d1e5bb0-1","hypotheses":[]}

Answer 1

There are a few issues with getTextFromJSON involving use of pointers, code duplication and use of an inappropriate function.

Pointers first. In this code:

jsontok_t tokens[15];
jsontok_t key;
...
key = tokens[i];

you are copying the token when you could be using a pointer:

jsontok_t *key = &tokens[i];

Copying is obviously more costly than pointing. Also note that it is often best to define the variable at its first point of use.

---

There are two points of duplication, one determining the size of the token array and one concerned with copying the string. Instead of this:

jsontok_t tokens[15];
...
parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)

I would do either:

#define N_TOKENS 15
jsontok_t tokens[N_TOKENS];
...
parseJson(&p, json, tokens, N_TOKENS);
for(unsigned int i = 0; i < N_TOKENS; ++i)

or

jsontok_t tokens[15];
#define N_TOKENS (sizeof tokens / sizeof tokens[0])  // or const int N_TOKENS inseatd of #define 
...etc 

The string copying code-duplication is actually unnecessary if you use a better standard library function...

---

Instead of using strcmp to compare the string (which involves constructing the string because it lacks a \0):

    if(!strcmp(keyString, "utterance"))

use memcmp. And you can use strndup to create a copy of the key. If you don't have strndup which is non-standard, you can easily write it (in a separate function).

These changes wont speed it up much, but are certainly cleaner:

char* getTextFromJSON(const char *json)
{
    if (!json) return NULL;

    json_parser p;
    #define N_TOKENS 15  // this normally would be at the start of the file
    jsontok_t tokens[N_TOKENS];

    initJsonParser(&p);
    int n = parseJson(&p, json, tokens, N_TOKENS);
    for (int i = 0; i < n; ++i) {
        jsontok_t *key = &tokens[i];
        if (!memcmp("utterance", &json[key->start], (size_t) (key->end - key->start))) {
            ++key;
            return strndup(&json[key->start], (size_t)(key->end - key->start));
        }
    }
    return NULL;
}

Answer 2

Improvements

Check if parseJson() returns an error code.

int err = parseJson(&p, json, tokens, TOKEN_NUMBER);
if (err)
{
       fprintf(stderr, "Error parsing JSON: %d\n", err);
       return NULL;
}

---

The code initializes all of the structures, but ->start and ->end will equal -1, which is why memcmp() will fail sometimes.

for (i = parser->toknext; i < num_tokens; i++) {
    jsmn_fill_token(&tokens[i], JSMN_PRIMITIVE, -1, -1);
}

Checking for a -1 value in ->start or ->end should be sufficient.

if ((key->end - key->start) < 0) return NULL;

---

The tokens[] array is uninitialized before it is passed to parseJson(), so once you iterate beyond the last token, the code is trying to run memcmp() on uninitialized nonsense address values. Initialize tokens[] to something and then check for that initialization value in the start/end fields during in the for() loop.

memset(&tokens, 0, sizeof(tokens));

---

During each iteration of the loop check for length zero to see if the token is actually valid before passing it to memcmp(). Bail out of the loop if the token has length zero.

if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;

---

Add a test case for a blank token.

if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;

---

Final Code

#define TOKEN_NUMBER 15
char* getTextFromJSON(const char *json)
{
        if (!json) return NULL;

        json_parser p;
        jsontok_t tokens[TOKEN_NUMBER];
        memset(&tokens, 0, sizeof(tokens));
        initJsonParser(&p);
        int err = parseJson(&p, json, tokens, TOKEN_NUMBER);
        if (err)
        {
                fprintf(stderr, "Error parsing JSON: %d\n", err);
                return NULL;
        }
        for(int i = 0; i < TOKEN_NUMBER; ++i)
        {
                jsontok_t *key = &tokens[i];
                if ((key->end - key->start) < 0) return NULL;
                if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;
                if (!memcmp("utterance", &json[key->start], (size_t) (key->end - key->start)))
                {
                        ++key;
                        return strndup(&json[key->start], (size_t) (key->end - key->start));
                }
        }
        return NULL;
}