Coding best practices

[Tangerino]

Some coding best practices as a suggestion.
Can we simplify the function and have one free and or returning point in a function?
Reduce the variable scope to be close to their usage,
Not to use unsafe functions like strcat?
Avoid magic numbers on mallocs.
I'll give an example:
Thanks
Carlos Tangerino

char *cJSONUtils_FindPointerFromObjectTo(cJSON *object, cJSON *target) {
    char *ret = NULL;
    if (object == target) {
        /* found */
        ret = cJSONUtils_strdup("");
    } else {
        cJSON *obj = NULL;          <<<------ REDUCE the scope
        int type = object->type;    <<<------ REDUCE the scope
        int c = 0;                  <<<------ REDUCE the scope
        /* recursively search all children of the object */
        for (obj = object->child; obj; obj = obj->next, c++) {
            char *found = cJSONUtils_FindPointerFromObjectTo(obj, target);
            if (found) {
                if (type == cJSON_Array) {
                    /* reserve enough memory for a 64 bit integer + '/' and '\0' */
                    size_t retLen = strlen(found) + 23;     <<<----- MAGIC NUMBER
                    ret = (char*) malloc(retLen);
                    // <<<<<------ SAFE snprintf
                    snprintf(ret,retLen,  "/%d%s", c, found); /* /<array_index><path> */ 
                } else if (type == cJSON_Object) {
                    size_t retLen = strlen(found) + cJSONUtils_PointerEncodedstrlen(obj->string) + 2;
                    ret = (char*) malloc(retLen);   <<<------- EASY TO READ & DEBUG
                    *ret = '/';
                    cJSONUtils_PointerEncodedstrcpy(ret + 1, obj->string);
                    strcat(ret, found);             <<<<-------- PREFER strncat instead
                }
                /* reached leaf of the tree, found nothing */
                free(found);    <<<<----- ONCE HERE
            }
        }
    }
    /* not found */
    return ret;                 <<<----- ONCE HERE
}

[FSMaxB]

Thanks for your feedback.

Sadly being compatible with C89 comes with some constraints. The safe functions like strncat etc. are not available, they have been introduced by C99, same goes for reducing the scope of variables. In C89 you have to declare all of your local variables at the beginning of a function. actually it is allowed to declare variables later, but they have to be in a new scope and before any statement, therefore your suggestion regarding the variable scope are valid C89.

Many of the magic numbers can be replaced by a sizeof("string literal"). That would make it much easier to see what's going on. Pull requests are welcome. Example:

/* reserve enough memory for a 64 bit integer + '/' and '\0' */
size_t retLen = strlen(found) + sizeof("/-9223372036854775808"); /* hm, now it is only 23, not 24 ... */

And I agree that less exit points from a function are desirable. But when making changes like that, you have to be really careful that you don't accidentally introduce new bugs. I'd be much more comfortable with these kinds of changes if there were proper unit tests with good test coverage, so I think this has a higher priority at the moment.

[FSMaxB]

I'm closing this for know. It is not forgotten! I've been refactoring a lot of code already and I always try to follow good practices to my abilities. cJSON_Utils still looks like a mess, but I (or somebody else?) will eventually be able to tackle that as well. At least before the 2.0 release.