Thread: What can I do to make this better?

I have to take a phone number in the form of (111) 111-1111 and display only the numbers. I must use the strtok function. This is the function I wrote to do this but am looking for something that is better and for someone to explain if I used a poor practice. My driver just sends an array that I assign the number above.

Code:

char *separate(char *string1)
{
    char *token;

    token = strtok(string1, "() -");
    string1 = token;

    while ((token = strtok(NULL, "() -")) != NULL)
    {
        *strcat(string1, token);
    }

    return string1;
}

Thanks

The assignment I'm working on requires me to use it.

1. That was a mistake, now updated. Thanks for that catch.

2. I know it overlaps the string but it doesn't quite disturb the token because it never catches up to it. That's why I was asking for a better way to do this with the requirements I have to abide by. This function does its job and returns the correct output otherwise but I am always looking for a more correct solution.

Thank you for all your replies.

Salem - Forgot to reply to your last question. There is no validation because it only takes a string I assign in the form (xxx) xxx-xxxx with all numbers. I want to send this string to my function above and remove the "() -" from it and either return this new string or change the actual string in my function so I don't have to return anything. Where is it stated that this is undefined behavior statement? I actually want to know so I can know where to look next time.

anon - Same as above, I'm not worried about validation right now as I send the string but thanks for the recommendation. Strange requirement because the book is fairly new but uses deprecated functions too, I agree there.

matsp - I'm trying to do as you suggested but am having some difficulty as how to implement it.

strtok saves a pointer to where the previous call of strtok ended; hence, fiddling with the string between calls to strtok can interfere with the whole process. Also, strcat's parameters are marked "restrict" which means they cannot refer to overlapping memory. In this case I'm thinking this happens: we start with "(111) 111-1111", strtok turns this into "(111\0 111-1111" and returns a pointer to the first 1, which string1 is assigned to. We call strtok again, which remembers where we are, so now we're looking at "(111\0 111\01111", the strcat turns this into "I'm a little teapot short and stout", and things go downhill from there. (Note that your version of strcat might handle this sort of copying correctly, in fact it probably does, but I don't think it has to, since where the material comes from and where it goes to share memory space.)

Salem - Thank you for your guidance. I wasn't aware of this being available as a pdf for free. It's actually a nice thing to have.

Elysia - I believe that you are correct but my original thought in mind was to post my main function including some c++ items. That was before I posted. Then the time came and I forgot to post it as I had planned. Sorry for the inconvenience. Your strtok function in your sig comes as a coincidence though. I'll have to study that code.

> I wasn't aware of this being available as a pdf for free.

It's just a draft, not the actual standard.

Sorry for taking so long but this is my new modified code because of the advice and warnings from all the above posters. The same rules apply: use strtok to remove everything but numbers from the form of (555) 555-5555
As originally stated if anything can be made better or if I am violating something such as undefined behavior and the like, then just let me know as I take all your inputs as valuable knowledge.

Code:

void separate(char *string1)
{
    char stringSep[15];
    char *token;

    strcpy(stringSep, string1);

    token = strtok(stringSep, "() -");

    strcpy(string1, token);

    while((token = strtok(NULL, "() -")) != NULL)
    {
        strcat(string1, token);
    }
}