Simple console JSON formatter

Question

I am writing simple formatter for valid JSON. Is reads JSON data from stdin and writes formatted output to stdout.

Goals:

• given valid input produce valid formatted JSON output
• constant memory usage
• minimal viable program (smallest program that solves a particular problem)
• feature free
• easy to read and understand
• be C99 compatible

Non-goals:

• validating JSON
• handling arguments

I am not aiming JSON validation right now (as said "formatter for valid JSON").

I tried adding arguments (like setting placeholder string with -p) but faced several issues:

1. unclear what arguments must be implemented
2. I don't feel comfortable yet writing argument parsing code.

So I skipped these for now.

Latest file in GitHub: https://github.com/sineemore/juu/blob/master/juu.c

#include <stdio.h>
#include <stdlib.h> /* EXIT_SUCCESS */

#define BUF_SIZE (1024 * 4)

int main(int argc, char **argv) {

    char buf[BUF_SIZE];
    const char placeholder[] = "  ";
    unsigned int indent = 0;
    unsigned int i = 0;
    unsigned int k = 0;
    char is_string = 0;
    char escaped = 0;
    char ch;
    size_t n;

    while (0 < (n = fread(&buf, sizeof(char), BUF_SIZE, stdin))) {
        for (k = 0; k < n; k++) {
            ch = buf[k];

            if (is_string) {
                /* Inside quoted string */
                putchar(ch);
                if (! escaped) {
                    if (ch == '"') {
                        /* Unescaped quote, string just ended */
                        is_string = 0;
                    } else if (ch == '\\') {
                        escaped = 1;
                    }
                } else {
                    escaped = 0;
                }
                continue;
            }

            switch (ch) {

            case ' ':
            case '\t':
            case '\n':
            case '\r':
                /* Ignoring original formatting */
                break;

            case '{':
            case '[':
                putchar(ch);
                putchar('\n');
                i = ++indent;
                while (i-- > 0) fputs(placeholder, stdout);
                break;

            case '}':
            case ']':
                putchar('\n');
                i = --indent;
                while (i-- > 0) fputs(placeholder, stdout);
                putchar(ch);
                if (indent == 0) putchar('\n');
                break;

            case ',':
                putchar(',');
                putchar('\n');
                i = indent;
                while (i-- > 0) fputs(placeholder, stdout);
                break;

            case ':':
                putchar(':');
                putchar(' ');
                break;

            case '"':
                /* String/property key start, see if clause on top (line 20) */
                putchar('"');
                is_string = 1;
                break;

            default:
                /* Numbers, true, false, null */
                putchar(ch);
                break;
            }
        }
    }

    return EXIT_SUCCESS;
}

Example output:

$ wget -qO- 'https://xkcd.com/info.0.json' | juu
{
  "month": "4",
  "num": 1979,
  "link": "",
  "year": "2018",
  "news": "",
  "safe_title": "History",
  "transcript": "",
  "alt": "HISTORIANS: We've decided to trim the past down to make things more manageable. Using BCE/CE, would you rather we lose the odd-numbered or even-numbered years?",
  "img": "https://imgs.xkcd.com/comics/history.png",
  "title": "History",
  "day": "11"
}

UPDATE:

• I missed ALL ferror() calls, fixing this part.
• Also program doesn't handle SIGPIPE, therefore it may be killed. Just tested it. I don't see a clear solution, should I set SIG_IGN?

Answer 1

We'll have a look at your code from the top to the bottom. The proper indentation makes that easy.

Magic numbers and defines

First of all, it's great that you've used BUF_SIZE instead of magic numbers, e.g.

char buf[1024 * 4]; // bad!

However, #defines can be error prone. You've used parentheses, which are often necessary. Furthermore, BUF_SIZE doesn't exist in your compiled program anymore, which can lead to some confusion if you want to debug. So consider the possible alternatives. In this case, a #define is fine. But there is no need for abbreviation:

//! Buffer size for reading from stdin.
#define BUFFER_SIZE (1024 * 4)

While we're at it, add some documentation. BUFFER_SIZE and buf[BUFFER_SIZE] are only some lines away from each other, but that might change later. The ! after // is Doxygen specific, you can ignore it if you don't use Doxygen.

Declarations and initializations

You use C99 and therefore can declare variables as late as you want. Whenever you declare a variable but set it a lot later, try to rewrite it as initialization at the right point. For example, ch isn't used until ch = buf[k]. We should keep it's scope limited. That way we cannot accidentally reuse variables.

If we follow this suggestion then i, k and ch get limited in their scope. We will have a look at that later, though. And since we already renamed BUF_SIZE to BUFFER_SIZE, we could also rename buf to buffer. You can of course choose other names, but again: there is no need to abbreviate. Disk space isn't expensive anymore, so choose names that you still understand after several months or years when someone calls you in the middle of the night.

Input and sizeof usage

fread(...,..., SIZE ,...) may not return SIZE. That can either happen if you're at the end of the file or if an error happens. You should check the FILE* with feof or ferror if that happens.

We stay at fread. While it's unlikely that you change buffer's type, it's usually good practice to use sizeof(*buf) or sizeof(buf[0]). If you ever change char buffer[BUFFER_SIZE] to mychar buffer[BUFFER_SIZE], you don't have to remember to change sizeof(char) to sizeof(mychar).

State machines and repetition

Your loop is essentially a state machine. The state machine itself looks fine. However, there is a lot of repetition. We have

while (i-- > 0) fputs(placeholder, stdout);

four times. That really asks for a function:

/**
 * \brief Puts the given \c str \c count times on \c stream.
 * \param str null-terminated character string to be written
 * \param count number of times \c str shall be written
 * \param stream output stream
 * \returns a non-negative value on success
 * \returns EOF on error and sets the error indicator
*/
inline void fputs_repeat(const char * str, size_t count, FILE * stream) {
    int value = 0;
    while (count-- > 0) {
        value = fputs(str, stream);
        if (value == EOF) {
            return EOF;
        }
    }
    return value;
}

Now we can just use fputs_repeat(placeholder, indentation, stdout) wherever you've used while (i-- > 0) .... We would now end up with the following variant:

#include <stdio.h>
#include <stdlib.h> /* EXIT_SUCCESS */

#define BUFFER_SIZE (1024 * 4)

inline void fputs_repeat(const char * str, size_t count, FILE * stream) {
    int value = 0;
    while (count-- > 0) {
        value = fputs(str, stream);
        if (value == EOF) {
            return EOF;
        }
    }
    return value;
}

int main(int argc, char **argv) {

    char buffer[BUF_SIZE] = {0};
    const char placeholder[] = "  ";
    unsigned int indent = 0;
    char is_string = 0;
    char escaped = 0;
    size_t n;

    while (0 < (n = fread(&buffer, sizeof(buffer[0]), BUFFER_SIZE, stdin))) {
        // exercise: add error handling
        for (unsigned int k = 0; k < n; k++) {
            char ch = buffer[k];

            if (is_string) {
                /* Inside quoted string */
                putchar(ch);
                if (! escaped) {
                    if (ch == '"') {
                        /* Unescaped quote, string just ended */
                        is_string = 0;
                    } else if (ch == '\\') {
                        escaped = 1;
                    }
                } else {
                    escaped = 0;
                }
                continue;
            }

            switch (ch) {

            case ' ':
            case '\t':
            case '\n':
            case '\r':
                /* Ignoring original formatting */
                break;

            case '{':
            case '[':
                putchar(ch);
                putchar('\n');
                fputs_repeat(placeholder, ++indent, stdout);
                break;

            case '}':
            case ']':
                putchar('\n');
                fputs_repeat(placeholder, --indent, stdout);
                putchar(ch);
                if (indent == 0) putchar('\n');
                break;

            case ',':
                putchar(',');
                putchar('\n');
                fputs_repeat(placeholder, indent, stdout);
                break;

            case ':':
                putchar(':');
                putchar(' ');
                break;

            case '"':
                /* String/property key start, see if clause on top */
                putchar('"');
                is_string = 1;
                break;

            default:
                /* Numbers, true, false, null */
                putchar(ch);
                break;
            }
        }
    }

    return EXIT_SUCCESS;
}

Output

You use putchar quite often. In some instances multiple calls can get replaced by puts or fputs. For example, instead of

putchar(',');
putchar('\n');

you could just use

puts(",");

and instead of

putchar(':');
putchar(' ');

you could use

fputs(": ", stdout);

Either way, if you're striving for performance, you want to keep the number of function calls low, so consider an output buffer if your current variant isn't fast enough for your liking. But first measure your program before you change it.

Goals

Let's revisit your goals and check them now.

• given valid input produce valid formatted JSON output

Since you don't introduce additional characters in strings and never remove characters from the original JSON except whitespace (outside of strings), you've reached that goal.

• constant memory usage

As there is only a single buffer, you've reached that goal too, although fputs might buffer.

• minimal viable program (smallest program that solves a particular problem)

Ah, that's a definition problem. What's minimal? What's "smallest"? Your program is short, but a single additional function removed some repetition which can lead to a technical debt. That function won't increase the size of your program, though.

• feature free

Check.

• easy to read and understand

The ! escaped logic took a little bit, but apart from that, goal reached.

• be C99 compatible

Yes, but use those features I've mentioned above (inline functions, late declarations).

Also program doesn't handle SIGPIPE, therefore it may be killed. Just tested it. I don't see a clear solution, should I set SIG_IGN?

If the input pipe is broken, the JSON input will suddenly end and you have invalid JSON. Do you need to handle invalid JSON at that point? It's a non-goal, as you said.

Answer 2

Answering my own question.

Error checking

Calls to fread, putchar and fputs doesn't test for EOF.

Check for fread error right after while loop:

if (ferror(stdin)) {
    return EXIT_FAILURE;
}

all calls to putchar and fputs may be replaced by wrappers:

static void outc(char ch) {
    if (EOF == putchar(ch)) {
        exit(EXIT_FAILURE);
    }
}

static void outs(const char *str) {
    if (EOF == fputs(str, stdout)) {
        exit(EXIT_FAILURE);
    }
}