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

> I must use the strtok function.
Why?

> *strcat(string1, token);
1. What's the * for at the beginning.
2. The bad thing here is you're using strcat to append to the same string you're trying to tokenise as well. Nearly all C standard library functions are undefined if the objects overlap like this.

Does something else validate the number as being a valid phone number?

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.

I would prefer a solution that copies to a new string. The best solution would be to pass in the string to copy into, along with the size of this string [there's obviously no way that your code can know that the telephone number input can fit in the output string unless you know the size].

--
Mats

> 2. I know it overlaps the string but it doesn't quite disturb the token because it never catches up to it.
No, you're assuming that because that's how your current compiler implements it.
That is the essence of what "undefined behaviour" means. Today, you're in the rose garden, tomorrow you're in the compost heap.

Implement your own "safe" version of strcpy which you know preserves the token position, then you'll be fine.

> I must use the strtok function.
Why?

That is indeed a strange requirement. Wouldn't it be so much easier simply to copy only digits - particularly as you are going to merge the tokens anyway (and perhaps report an error if any character besides digits and () - is encountered. Right now this function will accept "(xyz) abc-def" as a valid phone number too.

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.)

Originally Posted by c99

7.21.2.3 The strcpy function
Synopsis
1 #include <string.h>
char *strcpy(char * restrict s1,
const char * restrict s2);
Description
2 The strcpy function copies the string pointed to by s2 (including the terminating null
character) into the array pointed to by s1. If copying takes place between objects that
overlap, the behavior is undefined.
Returns
3 The strcpy function returns the value of s1.

Get your free copy of the final C99 Draft here - http://www.open-std.org/jtc1/sc22/wg14/www/docs/n869/


Also, there are several other categories of behaviour which you should be aware of when you really get into this stuff.
http://c-faq.com/ansi/undef.html

Although, this is the C++ board. Are you writing C or C++?
This code is all C.

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.

Well, the strtok is not the best implementation, but I thought it better than C's original strtok which is destructive.
But if you're going to write C++, you would need a copy of the C++ standard, and not C. That's the reason I asked.

> 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);
    }
}

Given your existing constraints on the input parameter, it seems fine.