C - Can not use strcpy with strtok

Question

I am working with a function that accept full name as input and print them out in order: first name, middle name, last name.

The problem is my while loop seem to go infinite and i haven't figured out how to fix it.

Here is my function:

void order(char ar[])
{
    int i, count=0, a=1;
    char* token;
    char last[20], middle[20], first[20],c;
    char s[]=" ";
    for(i=0;i<strlen(ar)-1;i++)
        if(ar[i]==' ') count++;

    token=strtok(ar,s);
    strcpy(last,token);


    while(token!=NULL)
    {
        token=strtok(NULL,s);
        if(a<count) strcpy(middle,token);
        else strcpy(first,token);
        a++; 
    }
    printf("%s %s %s",first,middle,last);
}

It is remarkably hard to read code laid out as shown — there are advantages to the orthodox layout styles, and one of them is readability and another is familiarity. — Jonathan Leffler, Jun 20 '18 at 05:03
What happens if there are several adjacent blanks between parts of the name, or at the beginning or end? You also don't check for overflows. But the principal problem is that you don't check for null quick enough. — Jonathan Leffler, Jun 20 '18 at 05:10
@JonathanLeffler is my code hard to read? I am new to coding, could you tell me how to make it easier to read. — Thuan Nguyen, Jun 20 '18 at 05:12
@JonathanLeffler this code assumes that users input correctly and how can check for null? i thought while(token!=NULL) checks for null already? — Thuan Nguyen, Jun 20 '18 at 05:15
Assuming that users do anything other than try to make your code crash is dangerous. They always get it wrong. You should assume they are doing their worst. Yes, `while (token != NULL)` checks for nullness, but you then have `token = strtok(NULL, s);` and you don't check `token` immediately after that — which is the crux of your problem, as pointed out by @Pras and @Soumen. (See also [Falsehoods programmers believe about names](http://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/).) — Jonathan Leffler, Jun 20 '18 at 05:25
@JonathanLeffler Thank you for spending time replying and helping me!! — Thuan Nguyen, Jun 20 '18 at 05:52
@JonathanLeffler really sorry but can i ask you 1 more question? What can i use instead of strlen()? Because in the main function i have a string: char ar[500] (which is the argument for the function i mentioned above). I use fgets for getting input and strlen is the only way that i know to calculate the number of elements in the inputted string. — Thuan Nguyen, Jun 20 '18 at 06:05
The length doesn't change during the `for` loop. Either use `int len = strlen(ar);` before the loop and then `i < len` in the loop, or use `for (int i = 0; ar[i] != '\0'; i++)` as the loop control. That avoids scanning the string twice — once for `strlen()` and once to look for blanks (and saving `len` saves scanning the string once per character in the string, which might save a lot, or which might be detected by a very good optimizer). — Jonathan Leffler, Jun 20 '18 at 06:11

score 1 · Answer 1 · answered Jun 20 '18 at 05:08

When first is copied (i.e. all tokens identified), while loop will be entered once again(note that token is not NULL, but points to first in this case). Now strtok() will be called and it will return NULL. Code will go to the else part and try to strcpy(), which is causing the problem.

Check for NULL before strcpy(). I have done this like this and it works fine.

while(token!=NULL)                                                          
{                                                                           
    token=strtok(NULL,s);                                                   
    if (token == NULL)                                                      
      break;                                                                
    if(a<count) strcpy(middle,token);                                       
    else strcpy(first,token);                                               
    a++;                                                                    
}

So if i try to copy NULL to a string, it will cause problem? And i know that strtok maintains a static pointer to continue from where it has left, but i don't understand why passing null pointer to the second call causes strtok continue to scan the string. Sorry for my grammar, if you don't understand, pls tell me and i will fix it. — Thuan Nguyen, Jun 20 '18 at 05:49
Providing NULL as argument to strcpy() results in segfault. You can check it using a simple program. And for how strtok(), after giving NULL argument, starts scanning where it left off, you would have to check the source code of strtok() itself. You can check [the source code](https://www.gnu.org/software/libc/sources.html) — Soumen, Jun 20 '18 at 08:12

score 1 · Answer 2 · answered Jun 20 '18 at 05:23

This is more like how I'd present the code in the question — before I passed it through a compiler:

void order(char ar[])
{
    int count = 0, a = 1;
    char *token;
    char last[20], middle[20], first[20];
    char s[] = " ";

    for (int i = 0; i < strlen(ar) - 1; i++)
    {
        if (ar[i] == ' ')
            count++;
    }

    token = strtok(ar, s);
    strcpy(last, token);

    while (token != NULL)
    {
        token = strtok(NULL, s);
        if (a < count)
            strcpy(middle, token);
        else
            strcpy(first, token);
        a++;
    }
    printf("%s %s %s", first, middle, last);
}

You can argue about brace placement — I use Allman but 1TBS is a widely used alternative style. I recommend choosing one of those two. I put braces around the body of a loop if it is more than one line; YMMV.

Use more white space around operators. Don't define unused variables (c vanished). I placed int i in the for loop. I would probably define one variable per line, and I probably wouldn't use the count or a.

The call to strlen() should not be in the condition of the loop. I'm not sure how you'd cope with my name — I don't have a middle name. I think you could avoid the while loop altogether. I'm not convinced you need the for loop either. Those are a separate discussion.

I'm not attempting to fix the algorithmic problem — this is not an answer to the question in the question (so it shouldn't be accepted), but is an answer to the question in the comment, and since code cannot be formatted in comments, I can't answer it sanely in a comment; hence, this 'not an answer' but 'intended to be helpful to an auxilliary question from the OP'.

C - Can not use strcpy with strtok

2 Answers2