HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcMinor

Parsing my JSON-like format in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
jsonformatparsinglike

Problem

For the past few weeks, I've been using the wonderful cJSON to add content to my game. But recently, I got tired of JSON's verbosity so I decided to roll my own format called SON (Simple Object Notation). I plan on using this format in all of my future game projects.

rogue
    anim
        timer 0
    brain
        type player
    image
        visibility visible
        texture rogue
        imagebox 0 0 24 40
        imageoff 0 -20
    player
    pos
        loc -30 85
        dir south
        solidity solid
kobold
    anim
        timer 0
    brain
        type monster
    image
        visibility visible
        texture kobold
        imagebox 0 0 24 40
        imageoff 0 -20
    pos
        loc 12 45
        dir south
        solidity solid


My parser is closely modeled after cJSON. I still have work to do (search by key, file writing, etc) but I'd like to get feedback on what I have so far. The three things I'm concerned about are:

  • How is my coding style? I've only been using C for a few weeks and I'm still trying to find what works for me. I used to write C++ capitalization in Allman but I've recently switched to C-style capitalization and 1TBS.



  • Is my file parsing efficient? This is my first time reading files in C and I struggled for several days figuring out the recursion part.



  • How would approach writing my SON tree back to a file? What algorithms do I need to learn? I want my output to look like the input character-for-character.



sonparse.h

```
#pragma once

/ A node in a son file with accompanying value and relatives /
typedef struct son son;

struct son {
char* key;
son* child;
son* next;

enum { SON_NONE, SON_INTEGER, SON_STRING, SON_ARRAY } type;
union {
int integer;
char* string;
struct {
int* data;
int size;
} array;
};
};

/ Create a new node and set pointers to NULL. /
son* son_new(void);
/ Recursively free a node and all of its relatives. /
v

Solution

Welcome to Code Review, nice first question. If you had more points I would
suggest putting a bounty on the question to get more answers.

Possible Bugs

The first possible bug is in son_new(), key is not initialized to NULL.
A good practice is to initialize all fields in an allocated structure, the previous value of the memory location is unknown.

The next possible bug is in son_parse(), the variable file is not checked to see if a it contains a valid file pointer. If fopen() fails the value of file will be NULL, and any use of the file pointer will result in a program crash. The Global Variable errno will provide an indication of why the call failed. The fread() and fwrite() functions can both fail.

Consider using intatoi(const char* string) rather than using the following code in read_array():

c = getc(file);
        while (c && c != '\n') {
            if (c == '-') {
                sign = -1;
            } else if (c == ' ') {
                ++i;
                sign = 1;
                node->array.data[i] = 0;
            } else {
                /* Add digit to current index. */
                node->array.data[i] *= 10;
                node->array.data[i] += (c - '0') * sign;
            }
            c = getc(file);
        }


One possible bug in the preceding code is that it may try to convert
non-numeric characters, there should at least be a check using isdigit(char c).

A Second possible bug in the preceding code is that the numeric value of the
converted string may be larger than the word size on the computer running the
program.

Coding Style

This question is somewhat subjective, you will usually get some comments or
answers here on Code Review if the style is bad or can be improved. As long
as you pick a coding convention such as camelCase or Capitalization and stick
with it throughout the code you're doing good. Your indentation is already
fine. Some people may have avoided answering the question because of the
subjectivity of the question.
If you are coding professionally the company or the department may have
coding standards that you must use (it makes it easier for other developers
to fix the code when you are on vacation or have gotten a better job).

Naming Conventions for Types, Functions and Variables

If the functions were more descriptively named, the need for comments would
be greatly reduced. Comments are generally used to describe what the
the algorithm is or why the specific algorithm was used. Using underscores
in a name does differentiate the word in the name but can take up a lot
more space. The Example Usage comment in son_parse is a good example of
what a comment should be. The comment / Check if a node was returned to this depth / is also a good comment.

Examples:

  • The struct son might be better named son_node, or SonNode.



  • The son_new function might be better named son_new_node or


SonNewNode.

  • The file variable in son_parse.c might be better named


SonInputFile().

Typedef's

The code in son_parse.h could be made more readable in this manner:

typedef enum { SON_NONE, SON_INTEGER, SON_STRING, SON_ARRAY } SonType;

typedef struct son_node {
    char* key;
    son* child;
    son* next;

    SonType type;
    union {
        int integer;
        char* string;
        struct {
            int* data;
            int size;
        } array;
    };
} SonNode;


For more object oriented pointers to functions in the struct can be added.
Some pointers to functions that might be added are:

  • AddChild()



  • DeleteChild()



  • AddNextNode()



  • DeleteNextNode()



  • WriteNodeToOutputFile()



  • PrintNode()



This would remove the necessity for the declarations of son_new(), son_del()
and son_print() in son_parse.h

Debug Code

Embed the debug code in #ifdef DEBUG

Example

in son_parse.h:

#ifdef DEBUG
    void son_print();
#endif


in son_parse.c

#ifdef DEBUG
void son_print(son* node) {
    printf("%s", node->key);

    /* Print node value, or (null) if none. */
    char* valuestr = NULL;
    switch (node->type) {
    case SON_INTEGER:
        printf(" : %d\n", node->integer);
        break;
    case SON_ARRAY:
        printf(" :");
        for(int i = 0; i array.size; ++i) {
            printf(" %d", node->array.data[i]);
        }
        putchar('\n');
        break;
    case SON_STRING:
        valuestr = node->string;
    case SON_NONE:
    default:
        printf(" : %s\n", valuestr);
        break;
    }

    /* Print node siblings, or (null) if none. */
    char* nextstr = NULL;
    char* childstr = NULL;
    if (node->next) {
        nextstr = node->next->key;
    }
    if (node->child) {
        childstr = node->child->key;
    }
    printf("next: %s child: %s\n", nextstr, childstr);
}
#endif


You can define DEBUG in the CC or gcc command line, have a DEBUG target in
a make file, or your IDE may provide a DEBUG mode that defines the DEBUG
macro for you.

Global Variable

Code Snippets

c = getc(file);
        while (c && c != '\n') {
            if (c == '-') {
                sign = -1;
            } else if (c == ' ') {
                ++i;
                sign = 1;
                node->array.data[i] = 0;
            } else {
                /* Add digit to current index. */
                node->array.data[i] *= 10;
                node->array.data[i] += (c - '0') * sign;
            }
            c = getc(file);
        }
typedef enum { SON_NONE, SON_INTEGER, SON_STRING, SON_ARRAY } SonType;

typedef struct son_node {
    char* key;
    son* child;
    son* next;

    SonType type;
    union {
        int integer;
        char* string;
        struct {
            int* data;
            int size;
        } array;
    };
} SonNode;
#ifdef DEBUG
    void son_print();
#endif
#ifdef DEBUG
void son_print(son* node) {
    printf("%s", node->key);

    /* Print node value, or (null) if none. */
    char* valuestr = NULL;
    switch (node->type) {
    case SON_INTEGER:
        printf(" : %d\n", node->integer);
        break;
    case SON_ARRAY:
        printf(" :");
        for(int i = 0; i < node->array.size; ++i) {
            printf(" %d", node->array.data[i]);
        }
        putchar('\n');
        break;
    case SON_STRING:
        valuestr = node->string;
    case SON_NONE:
    default:
        printf(" : %s\n", valuestr);
        break;
    }

    /* Print node siblings, or (null) if none. */
    char* nextstr = NULL;
    char* childstr = NULL;
    if (node->next) {
        nextstr = node->next->key;
    }
    if (node->child) {
        childstr = node->child->key;
    }
    printf("next: %s child: %s\n", nextstr, childstr);
}
#endif
#pragma once

Context

StackExchange Code Review Q#149466, answer score: 5

Revisions (0)

No revisions yet.