6
\$\begingroup\$

I am trying to improve my skill in C, and this time by getting away from strtok. The following code splits the input string into a token_list, an array of strings.

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

#define MAX_LINE_LEN 256
#define NULL_TERM '\0'

/* A function that will print the content of the token_list */
int printcharlist(char **tok_list)
{
    char **pptr = tok_list;

    while (*pptr) {
        printf ("++ %s\n", *pptr);
        pptr++;
    }

    return 0;
}

/* returns the string between 2 pointers (not forgetting to append NULL_TERM */
char *getStr(char *start, char *end)
{
    int length=end-start;
    int i=0;
    char *result = (char *)malloc (sizeof (char) * (length+1));

    while (i < length) {
    result[i] = *start;
    start++;
    i++;
    }

    result[i] = NULL_TERM;

    return result;
}


int main (int argc, char ** argv)
{
    char *input = "something;in;the;way;she;moves";
    char reject = ';';

    char *start = input;
    char *end = input;

    char *token_list[MAX_LINE_LEN] = {NULL};
    int i=0;

    /* Is this predicate okay? I read that certain compiler might raise warnings here */
    while (end=strchr(start, (int)reject)) {

        token_list[i]=getStr(start, end);

        i++;
        end++;
        start=end;
    }

    /* It feels awkward to add one more token after the while loop. Is it standard? Should/can I avoid this? */ 
    token_list[i]=start;
    printcharlist(token_list);

    return EXIT_SUCCESS;
}

I am interested in any kind of feedback I can get. But if I absolutely must ask questions it would be:

  • Would something make you react badly if someone sent you this code in a patch/pull-request? If yes, what?
  • Consider this code is going to production. What are the edge cases that I miss and how can I solve them?
\$\endgroup\$
6
\$\begingroup\$

Module Design

The trouble with your design is that you need to know how it works before you can use it. This is not true for strtok (OK to a slight extent it is true for strtok).

When designing a software system the user should not need to know how it operates, but rather the interface to the system. Doing it this way has two distict advantages.

  1. User does not need to learn how it works thus making it easier for them to use.
    This means it is easier to distribute in binary form if you want.
  2. User is less likely to break it by making assumptions that do not hold (as they have not read the code to understand it).
  3. You can change how it works without affecting the user code.

Thus I would have provided this functionality with a couple of functions and some opaque data types that the user never needs to know or understand.

String Copying

while (i < length) {
result[i] = *start;
start++;
i++;
}

This is done in the std lib look up strncpy().

Also this can be written much more succinctly as:

for(loop = 0; loop < length; ++loop)
{
   result[loop] = start[loop];
}

Code Review

I don't think you need to define this.

#define NULL_TERM '\0'

Putting the literal '\0' into your code is very readable and people know what it means. The macro NULL_TERM may seem self explanatory but I would go look it up.

The function printcharlist assumes the list is NULL terminated.

int printcharlist(char **tok_list)

This is asking for people to do it incorrectly (and forget the NULL terminator). You even mention yourself that this is akward:

/* It feels awkward to add one more token after the while loop. Is it standard? Should/can I avoid this? */

I would change the interface to pass the number of values in the list:

int printcharlist(char **tok_list, size_t count)

Then you make people explicitly fill in the length and they can not accidentally pass you an array that will cause problems.

Alternative design

One advantage of strtok() of your function is that it does not parse the whole string. It only fetches the next token. Thus in a situation where you only need the first couple of tokens it is much more efficient.

I would change your algorithm so that it behaves in a similar manor. Retrieve one string (and store it in your data structure). Next call retrieves the next string etc (this would be a lot easier to change it you had made a modular design).

Questions

Would make you react badly if someone sent you this code in a patch/pull-request? If yes, what?

No. But I would want to see the test cases the validate it.

Consider this code is going to production. What are the edge cases that I miss and how can I solve them?

I would set up some unit tests.

  • ""
  • ";"
  • ";;"
  • ";;;"
  • "Some Text;"
  • "Some Text;;"
  • ";;SomeText;;"
  • "Some\nText"
  • "Some\nText;"
  • "Some\nText;;"
  • "Some\n;Text"
  • "Some\n;;Text"
  • "Some;\n;Text"
  • "Some;\n;;Text"
  • "Some;;\n;Text"
  • "Some;;\n;;Text"
\$\endgroup\$
  • 1
    \$\begingroup\$ strncpy will check for \0 as it copies and is inefficient. memcpy is best here, as we know the length. \$\endgroup\$ – William Morris May 24 '12 at 23:59
4
\$\begingroup\$

General comment:

  • leave a space around operators ('-', '=', etc)

printcharlist:

  • pptr seems redundant - just use tok_list
  • return void (ie nothing)

getStr:

  • inconsistent function naming (camel case, cf. printcharlist)
  • start and end should be const
  • don't cast the return from malloc
  • sizeof(char) is 1 by definition, so omit
  • replace while loop with memset
  • use '\0' directly

main

  • the guts of this should be a function separate from main().
  • input should be const
  • reject should be const (and perhaps 'int')
  • while() loop needs extra brackets as you hinted
  • instead of end++; start = end; I would use just start = end + 1; as the value of end is reassigned next loop.
  • you do not check for the list filling up
  • your trailing assignment of token_list[i]=start; probably worries you because it leaves token_list holding one statically allocated string and the reset dynamic; use strdup() to allocate the last string. You could avoid this trailing assignment by rearraging the logic of the function, but why bother? It is fine as it is.
\$\endgroup\$
  • \$\begingroup\$ The standard only guarantees that 8 bits fit into a char. There are architectures where char is bigger than 8 bits. If you want something that is exactly 8 bits, use stdint's uint8_t. On an architecture where it's unavailable, it simply won't compile. \$\endgroup\$ – Kuba Ober Jun 14 '12 at 3:53
4
\$\begingroup\$

I would consider changing the getStr() function a little, as follows:

char *getStr(char *start, char *end)
{
    int length=end-start;
    if(length <= 0) {
        /* Consider an error msg */
        return NULL;
    }

    char *result = (char *)malloc (sizeof (char) * (length+1));

    /* deliberately not using strcpy, since it checks for \0 */
    memcpy(result, start, length);
    result[length] = NULL_TERM;

    return result;
}

And a few minor aesthetic changes in main:

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

    int i = 0;
    while (end=strchr(start, (int)reject)) {
        token_list[i++]=getStr(start, end++);
        start=end;
    }

    /* Nothing wrong assigning the last token here, as it avoids more
     * complex logic in the while loop and simplifies the code, just
     * add a comment, something like: Now for the last token...
     */
    token_list[i]=start;
    printcharlist(token_list);

    return EXIT_SUCCESS;
}

If you're worried about the while() loop syntax, you could consider a for loop instead, but the way you have it is clean.

You may also consider a different implementation. Currently you are effectively duplicating the input string, which for relatively short strings, that shouldnt be problematic. But if the input strings become longer, you could implement an array of pointers to an intermediate struct defined as follows:

struct token {
    char *data;
    unsigned int length;
};

Each data pointer would point to the beginning of a token, and the length would be the length of said token. This way you wont have to duplicate the input string, and you wouldnt have to modify the input string either.

\$\endgroup\$
  • \$\begingroup\$ Thank you for mentioning memcpy. On the other hand, your second piece of code seems a little odd. token_list[0] gets assigned the whole input string and the final token is never assigned to the token_list array... \$\endgroup\$ – rahmu May 23 '12 at 9:44
  • \$\begingroup\$ @rahmu, Ok, I see your point. I think I originally mis-read your code. I was assuming the assignment after the while loop was to token_list[0], but now I see its actually to token_list[i]. I'll update the answer accordingly. \$\endgroup\$ – Brady May 23 '12 at 9:49
  • \$\begingroup\$ Thanks! The token struct is pretty neat indeed (although for my practical use case, input will never be too long so I would mind duplication). Thank you also for confirming that the final assignement is not as awkward as it seemed to me :) \$\endgroup\$ – rahmu May 23 '12 at 12:17
  • \$\begingroup\$ @WilliamMorris, you're absolutely right! Oversight on my part :) I updated the answer. \$\endgroup\$ – Brady May 25 '12 at 6:57
  • \$\begingroup\$ One thing wrong with the code is that you don't free the memory which you allocated in getStr. \$\endgroup\$ – user36370 Feb 7 '14 at 0:28

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.