4
\$\begingroup\$

I have created some code to split strings. The code will split the given string by the given delimiter into an array which is inside a struct it returns.

I am using strtok to do the actual tokenizing, but in my code the original string will not be changed.

split.h

#ifndef SPLIT_H
#define SPLIT_H

#ifdef __cplusplus
extern "C" {
#endif

    struct splitString {
        char **tokens;
        unsigned total;
        unsigned size;
    };

    struct splitString *split(const char *, const char *);
    void splitFree(struct splitString **);

#ifdef __cplusplus
}
#endif

#endif /* SPLIT_H */

split.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "split.h"

#define DEFAULT_TOKEN_SIZE 10

static struct splitString* initSplitString() {
    struct splitString *s = malloc(sizeof (struct splitString));

    if (s) {
        s->total = 0;
        s->size = DEFAULT_TOKEN_SIZE;
        s->tokens = malloc(sizeof (char *) * DEFAULT_TOKEN_SIZE);

        for (int i = 0; i < DEFAULT_TOKEN_SIZE; i++) {
            s->tokens[i] = NULL;
        }


        return s;
    }

    return NULL;
}

/**
 * Adjust the size of the tokens variable if necessary
 * @param s
 */
static void adjustSize(struct splitString *s) {
    if (s->total == s->size) {
        s->size += DEFAULT_TOKEN_SIZE;

        char **temp_tokens = realloc(s->tokens, sizeof (char *) * s->size);

        if (temp_tokens) {
            s->tokens = temp_tokens;
        }
    }
}

static void clearAndCopy(char *destination, const char *source, unsigned size) {
    memset(destination, 0, size);
    memcpy(destination, source, size);
}

struct splitString* split(const char *string, const char *delimiter) {
    unsigned size = strlen(string) + 1;
    char tmp[size];
    char *token = NULL;
    struct splitString *s = initSplitString();

    if (s == NULL) {
        goto error;
    }

    clearAndCopy(tmp, string, size);

    token = strtok(tmp, delimiter);

    while (token != NULL) {
        int bsize = strlen(token) + 1;

        s->tokens[s->total] = malloc(bsize);

        if (s->tokens[s->total]) {
            clearAndCopy(s->tokens[s->total], token, bsize);
        }
        else {
            splitFree(&s);
            goto error;
        }

        s->total++;

        adjustSize(s);
        token = strtok(NULL, delimiter);
    }

    return s;

error:

    return NULL;
}

/**
 * Clear all token and the splitString
 * @param s
 */
void splitFree(struct splitString **s) {
    for (int i = 0; i < (*s)->total; i++) {
        free((*s)->tokens[i]);
        (*s)->tokens[i] = NULL;
    }

    free(*s);
    *s = NULL;
}

Here is just some code I am using to test:

#include <stdio.h>
#include <stdlib.h>
#include "split.h"

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

    char *test_string = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";
    struct splitString *test = split(test_string, ",");

    printf("Total: %d\n", test->total);

    for (int i = 0; i < test->total; i++) {
        printf("String: %s\n", test->tokens[i]);
    }

    splitFree(&test);

    return EXIT_SUCCESS;
}
\$\endgroup\$
5
\$\begingroup\$

Unnecessary code

This function is puzzling to me:

static void clearAndCopy(char *destination, const char *source, unsigned size) {
    memset(destination, 0, size);
    memcpy(destination, source, size);
}

Why do you need to clear the destination buffer when you are going to copy over it immediately afterwards? All calls to this function could simply be replaced by calls to memcpy instead.

Memory leak

In split_free(), you forgot to free (*s)->tokens.

Allocation strategy

There are two things I would do differently with respect to how you allocate your tokens:

  1. Instead of using realloc to dynamically resize your token array, I would prescan the input and determine how many tokens there will be, and just allocate once. This avoids a possible \$O(n^2)\$ reallocation time.

  2. Instead of allocating a buffer for each token and copying the token to it, I would make a single copy of the input string, and then have s->tokens[i] be pointers to the tokens within the single copy.

\$\endgroup\$
  • \$\begingroup\$ Fixed the memory leak. Thanks! I will try to work on your other comments \$\endgroup\$ – AntonioCS Dec 13 '15 at 18:57
3
\$\begingroup\$

Always try to avoid goto whenever possible:

if (s == NULL) {
    goto error;
}

else {
    splitFree(&s);
    goto error;
}

The label just leads to return NULL, so you can just put that in place of each goto:

if (s == NULL) {
    return NULL;
}

else {
    splitFree(&s);
    return NULL;
}

If you want to show that this function returns NULL on error, then add proper documentation to clarify this. Using goto instead is not the right way of doing this.

\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Jamal Dec 12 '15 at 23:18

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.