7
\$\begingroup\$

I'm making a server in C that needs to parse JSON that follows oBIX protocol and I'm using the Jansson library for this. I've been programming in high level languages such as Java and C# for my entire career and the last time I touched C was way back in 7-8 years ago, so I'm mostly looking for guidance on writing C-style code.

Here's a sample JSON input from client to server:

{"obix":"obj","is":"obix:Request","rid":"1","children":[{"obix":"op","name":"add","is":"obix:Watch","children":[{"obix":"obj","is":"obix:WatchIn","children":[{"obix":"list","names":"hrefs","children":[{"obix":"uri","val":"/device/"}]}]}]}]}

So a function I have just needs to validate what kind of request the client is making. First, I write the following defines:

#define OBIX_REQUEST_INVALID -1
#define OBIX_REQUEST_WATCH 0
#define OBIX_REQUEST_UPDATE 1 
....

Then in my callback function. I first validate whether it's a valid JSON input.

int callback_web_socket(struct libwebsocket_context * this,
                                   struct libwebsocket *wsi,
                                   enum libwebsocket_callback_reasons reason,
                                   void *user, void *in, size_t len)
{
    ....

    json_t *root;
    json_error_t error;

    root = json_loads(buf,0,&error);
    if (!root)
    {
         printf("\nInvalid JSON data.\n");
         break;
    }
    ....

If it's valid JSON, I call my parse function to determine request type

int request_type = obix_parse_request(root);
printf("\nRequest Type: %d\n", request_type);

And here's how my parse function looks like:

int obix_parse_request(const json_t *obix_root_obj) {
    json_t *obix_obj = json_object_get(obix_root_obj,"obix");

    if (!json_is_string(obix_obj) || strcmp(json_string_value(obix_obj),"obj") != 0) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child = json_object_get(obix_root_obj, "children");
    if (!first_child || !json_is_array(first_child)) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child_obj = json_array_get(first_child, 0);
    if (!first_child_obj || !json_is_object(first_child_obj)) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child_is_str = json_object_get(first_child_obj, "is");
    if (!first_child_is_str || !json_is_string(first_child_is_str)) {
        return OBIX_REQUEST_INVALID;
    }

    const unsigned char *request_type = json_string_value(first_child_is_str);
    if (strcmp(request_type, "obix:Watch") == 0) {
        return OBIX_REQUEST_WATCH;
    }

    ...
}

If I were coding in C#, this function would raise a huge red flag for lots of code duplication. But this is C, and I'm readily assuming the code duplication is part of my incompetency in C language.

How would you elegantly write this function? Aside from cleaning up the function, I'd like to hear some comments on my coding style.

\$\endgroup\$
  • \$\begingroup\$ It is better to use const and enums than #define, because #define's are I scoped and unchecked. \$\endgroup\$ – user8128167 Nov 6 '13 at 19:23
  • \$\begingroup\$ Also, at compile time the preprocessor macros get replaced by their numerical value in the code, so it will be difficult to determine what a number is all about when debugging. \$\endgroup\$ – user8128167 Nov 6 '13 at 19:26
  • \$\begingroup\$ I would Lex and yacc it makes it very simple. \$\endgroup\$ – Martin York Nov 9 '13 at 17:15
10
\$\begingroup\$

A few notes:

  • My first concern with your program is the use of the Jansson library. Looking at some benchmarks shows me that it is less than stellar. (The Y-axis is parses-per-second. Thus, higher is faster.)

    So a new library choice may be in order. I would highly recommend looking a jsmn. I have yet to find a faster (and more portable!) option out there. Here is a benchmark for jsmn compared to Jansson (jsmn is 19.28 times faster).

    \$ \newcommand\T{\Rule{0pt}{1em}{.5em}} \begin{array}{|c|c|c|c|c|c|} \hline \textbf{Project} & \textbf{License} & \textbf{API} & \textbf{Neg. tests} & \textbf{Followers} & \textbf{Benchmark} \T \\\hline \text{Jansson} \T & \text{MIT} & \text{DOM} & \text{yes} & \dfrac{234}{59} & 25 \text{MB/s}, 52 \text{s} \\\hline \text{jsmn} \T & \text{MIT} & \text{custom} & \text{yes} & \dfrac{65}{4} & 482 \text{MB/s}, 2.3 \text{s} \\\hline \end{array} \$

  • You have used the preprocessor to define some oBIX request values.

    #define OBIX_REQUEST_INVALID -1
    #define OBIX_REQUEST_WATCH 0
    #define OBIX_REQUEST_UPDATE 1
    

    You could use an enum here instead.

    typedef enum 
    {
        INVALID = -1,
        WATCH = 0,
        UPDATE = 1
    } ObixRequest;
    
  • You can remove any != 0 tests from your if conditions for maximum C-ness.

  • You sometimes use printf() when you don't have to.

    printf("\nInvalid JSON data.\n");
    

    Use puts() for statements like this when you aren't formatting the string. As a plus, you don't have to include the \n characters.

    puts("Invalid JSON data.");
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.