Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to improve my C skills, and I hope someone might be able to provide me with some feedback on the following code. I avoid strtok function intentionally.

#define MAX_SIZE 1000
int split_up(char* string, char* argv[])
{
    char* p = string;
    int argc = 0;

    while(*p != '\0')
    {
        while(isspace(*p))
            ++p;

        if(*p != '\0')
            argv[argc++] = p;
        else
            break;

        while(*p != '\0' && !isspace(*p))
            p++;
        if(*p != '\0')
        {
            *p = '\0';
            ++p;
        }

    }

    return argc;
}

int main()
{
    char *av[MAX_SIZE];
    char string[] = "this is a test";
    //int i, ac = makeargv(string, av, 2);
    int i;
    int ac = split_up(string, av);
    printf("The number of token is: %d\n", ac);
    for(i = 0; i < ac; i++)
        printf("\"%s\"\n", av[i]);

    return 0;
}
share|improve this question
    
The same affect can be achieved using sscanf(). Just use a string format of " %*s" (note the leading space in the string). –  Loki Astari Jan 20 '13 at 15:34
add comment

1 Answer

up vote 1 down vote accepted

Your code looks quite nice, but here are a few nitpicking comments, for what they are worth.

  • Missing headers, but I assume you just didn't paste them.
  • split_up should be static and get a better name
  • variable names are odd. argc/argv are the usual names for main() (missing here) but not for functions. I'd use tokens instead of argv, and maybe n instead of argc. Okay, n is not at all descriptive, but for such a short function it need not be.
  • assigning string to p gains nothing, as the original value is not required later on.
  • perhaps use braces, even for 1-line statements following while, for, if etc
  • I prefer to leave a space after keywords (while, if, etc)
  • when breaking out of the loop, why not do it immediately:

    if (*p == '\0') {
        break;
    }
    argv[argc++] = p;
    
  • The while loop following this knows that *p is initially not \0, so put the check for that 2nd. Also it is better to add brackets round conditions, even if you think the precedence is obvious:

    while(!isspace(*p) && (*p != '\0')) {
        p++;
    }
    
  • you could also just check for *p instead of *p == '\0'. But many will disagree.

  • you also have two blank lines near the end of split_up that make the code look a little messy.

Note also that you don't check for overflow of the argv array. Okay, you gave it 1000 places for the purpose of the example and that is unlikely to overflow. But in practice you will use somewhat less than 1000 and overflow will be more likely.

And finally, my preference is not to nest loops. Your two nested while loops could be extracted into inline functions - they might be useful in themselves, after all. Here is an example with the loops extracted; you don't have to like it - it is just an example. I find this easier to read, but yours might well be slightly quicker, if that matters.

static inline char* next_token(char *p)
{
    while (isspace(*p)) {
        ++p;
    }
    return *p ? p : NULL;
}

static inline char* next_space(char *p)
{
    while (!isspace(*p) && *p) {
        ++p;
    }
    return *p ? p : NULL;
}

static int string_split(char *p, char* token[])
{
    int n = 0;
    while ((p = next_token(p)) != NULL) {
        token[n++] = p;
        if ((p = next_space(p)) == NULL) {
            break;
        }
        *p++ = '\0';
    }
    return n;
}

Or you could consider using strspn and strcspn and define your own whitespace characters...

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.