Usage of strtok()

Question

I tried my insertItem() list api inserting all types of items, successfully, for example, strings(below),

static void copyData(List *records, char *delim, char *argv[]){

char *item = malloc(5);
    strcpy(item, "abcd");
    int count = 0;
    while(count++ < 10){

      insertItem(records, item);

      item = malloc(5);
      strcpy(item, "efgh");
    }
}

---

Output:

$ ./frequencyCounter.exe ARRAY
Key is:abcd
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Key is:efgh
Size of list: 10
Before freeList()After freeList()

---

But strtok() tokens shows problem code, as shown below, buf[] parameter not required above,

static void copyData(List *records, char buf[], char *delim, char *argv[]){

  char *token =  strtok(buf, delim);

  void *item = malloc(strlen(token) + 1);
  item = memcpy(item, token, strlen(token)+1);
    printf("token-%s\n", token);        
  while(token != NULL){

    insertItem(records, item);
    token = strtok(NULL, delim);
    if(token != NULL){
      printf("token-%s\n", token);        
      item = malloc(strlen(token) + 1); //every item has its own heap memory
      memcpy(item, token, strlen(token)+1);
    }
  }

}

---

Output is,

$ ./frequencyCounter.exe ARRAY
token-it
token-was
token-the
token-best
token-of
token-times
Key is:it
Key is:it
Key is:it
Key is:it
Key is:it
Key is:it
Size of list: 6
Aborted (core dumped)

---

In the problem code(above), Can we re-use the result of strtok()?

Answer 1

Although it's quite hard to understand or read your code, the following looks bad

item = malloc(strlen(token));

you then memcpy() strlen(token) + 1 bytes.

This will cause undefined behavior and thus your program will behave erratically.

Also, don't overuse strlen() it iterates through all the characters of the input string every time you call it, instead just store the result somewhere if you will need to use it more than once.

This is an improved version of your function

static void 
copyData(List *records, char buf[], char *delim, char *argv[])
{
    char *token;
    size_t length;
    void *item;

    token = strtok(buf, delim);
    if (token == NULL)
        return;
    length = strlen(token);
    item = malloc(length + 1);
    if (item == NULL)
        return;
    memcpy(item, token, length + 1);

    printf("token-%s\n", token);        
    while (token != NULL) {
        insertItem(records, item);

        token = strtok(NULL, delim);
        if (token != NULL) {
            length = strlen(token);

            item = malloc(length + 1); // every item has its own heap memory
            if (item != NULL) {
                memcpy(item, token, length + 1);
            }
            printf("token-%s\n", token);
        }
    }
    // Some day, you will have to free these pointers
}

But I hope it's not just like this, because every item you malloc() will never be freed and also, because it violates the DRY principle somewhat. And one more thing, you can use strdup() too.