Split a string based on a delimiter in C

Question

Basically, this code splits a string into substrings based on a token. The printf call is just a placeholder for something useful for each substring and can be substituted for something else, like a spell checker, profanity checker, etc...

With the input ("The quick brown fox jumps over the lazy dog", 44, ' ') the output is:

Processed: The
Processed: quick
Processed: brown
Processed: fox
Processed: jumps
Processed: over
Processed: the
Processed: lazy
Processed: dog

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sal.h>

/**
 * Splits a string into substrings delimited by a token.
 * Returns the number of substrings found in the string.
 */
size_t __cdecl splitstring(
    _In_reads_or_z_(len) char *str, 
    _In_ size_t len, 
    _In_ char token
)
{
    char *temp = malloc(len + 1), *p = str, *q = str;
    memset(temp, 0, len);
    size_t i, cnt = 0;
    if (!temp)
    {
        _wperror(L"malloc");
        exit(EXIT_FAILURE);
    }

    for (i = 0; i < len + 1; i++)
    {
        if (str[i] == token || str[i] == '\0')
        {
            q = &str[i];
            strncpy_s(temp, len, p, q - p);
            printf("Processed: %s\n", temp);
            memset(temp, 0, len);
            cnt++;
        }
        // Useful property of boolean expressions in C: they are always either 1 or 0
        // Adjust p to point either to the next space or the next letter depending on if
        // it's the first word being processed.
        p = q + (p != &str[0]); 
        if (!*p) break;
    }

    free(temp);
    temp = NULL;

    return cnt;
}

Answer 1

Check allocated pointers before using them

It's good that we have a if (!temp) check, but unfortunately we used temp as argument to memset() before we did the check, so it's too late!

We could (and should) use calloc() if we need to zero out the memory we allocated. In this case, it's not necessary (we'll be overwriting it anyway), so we should just omit the memset() call.

Use const where appropriate

The pointers str, p and q can all point to const char; that allows us to safely pass a string literal as argument, for example, and it helps users understand what guarantees they are given.

Document the arguments

The meaning of the argument len isn't obvious from its name or from the function's comment. It seems that it's the largest substring we expect to handle.

Even better would be to make the code adapt (using realloc(), perhaps) to the string it's given, rather than requiring the user to specify that. I'll leave that as an interesting exercise for you.

Try not to mix indexing and pointer arithmetic

It's a little confusing to index into str using i, but to also have pointers p and q pointing into the string, too.

The break at the end of the loop

Since p initially points to the beginning of the string, we can move the test at the end of the loop to be part of the loop condition. This makes the loop structure easier to follow:

for (size_t i = 0;  i <= len && *p;  ++i)

(I changed the test of i slightly, for simplicity and also as an almost-automatic aversion to integer overflow).

We don't need a loop to find the next instance of token

We could use strchr() to find the next token in str. On GNU platforms, there's also strchrnul() which returns a pointer to the terminating NUL (instead of a null pointer) when the character isn't present.

Consider discarding empty words

Many users will expect the same result from "foo bar" and "foo bar" - it's a surprise to find that this code considers there to be a zero-length word between the two spaces in the latter string. Even more surprising is " foo" which splits to "" and " foo".

---

Reworked code

I started again from scratch, taking into account these observations. The interface is a bit different, as in real life we want to be able to re-use the function to perform different actions on the tokens found, so we accept a function and a state pointer for the function.

This is standard portable C - you're of course able to add your annotation header to this if you want.

#include <stdlib.h>
#include <string.h>

/**
 * @brief Call a function for each token found in the input string.
 * @param s input string
 * @param sep nul-terminated set of delimiter characters, or NULL for default
 * @param f callback function
 * @param data callback data
 * @return true if successful, false on any failure
 */
int process_tokens(const char *s, const char *sep,
                   void (*f)(const char*,void*), void *data)
{
    if (!sep) {
        /* use default word separators */
        sep = " \t\n.!?()";
    }

    char *buf = NULL;
    size_t buf_len = 0;

    while (*(s+= strspn(s, sep))) {
        /* s is now pointing at next word character */
        size_t word_len = strcspn(s, sep);

        /* ensure we have sufficient storage */
        if (buf_len <= word_len) {
            buf_len = 2 * word_len + 1;
            char *t = realloc(buf, buf_len);
            if (!t) {
                free(buf);
                return 0;
            }
            buf = t;
        }

        /* copy word into storage, and call user function */
        memcpy(buf, s, word_len);
        buf[word_len] = 0;
        f(buf, data);

        /* advance to next separator */
        s += word_len;
    }

    free(buf);
    return 1;
}


/* A simple test program */

#include <stdio.h>

static void print_token(const char *s, void *data)
{
    size_t *count = data;
    ++*count;
    printf("Processed: \"%s\"\n", s);
}

int main()
{
    size_t count = 0;
    const char *input = "  Pack my box  with five-dozen liquor jugs.  ";
    if (!process_tokens(input, NULL, print_token, &count)) {
        fprintf(stderr, "Error splitting input string\n");
        return 1;
    }

    printf("Processed %zu tokens\n", count);
}