What can I do to make this better?

This is a discussion on What can I do to make this better? within the C++ Programming forums, part of the General Programming Boards category; I have to take a phone number in the form of (111) 111-1111 and display only the numbers. I must ...

  1. #1
    Registered User
    Join Date
    Mar 2006
    Location
    Honolulu, HI/United States
    Posts
    12

    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

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,672
    > 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?
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User
    Join Date
    Mar 2006
    Location
    Honolulu, HI/United States
    Posts
    12
    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.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,672
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  6. #6
    The larch
    Join Date
    May 2006
    Posts
    3,573
    > 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.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  7. #7
    Registered User
    Join Date
    Mar 2006
    Location
    Honolulu, HI/United States
    Posts
    12
    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.

  8. #8
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    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.)

  9. #9
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,672
    Quote 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
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  10. #10
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,789
    Although, this is the C++ board. Are you writing C or C++?
    This code is all C.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  11. #11
    Registered User
    Join Date
    Mar 2006
    Location
    Honolulu, HI/United States
    Posts
    12
    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.

  12. #12
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,789
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  13. #13
    Registered User
    Join Date
    Oct 2001
    Posts
    2,129
    > I wasn't aware of this being available as a pdf for free.

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

  14. #14
    Registered User
    Join Date
    Mar 2006
    Location
    Honolulu, HI/United States
    Posts
    12
    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);
        }
    }

  15. #15
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,672
    Given your existing constraints on the input parameter, it seems fine.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Establishing 'make clean' with GNU make
    By Jesdisciple in forum C Programming
    Replies: 9
    Last Post: 04-11-2009, 09:10 AM
  2. How to make a Packet sniffer/filter?
    By shown in forum C++ Programming
    Replies: 2
    Last Post: 02-22-2009, 08:51 PM
  3. HELP!wanting to make full screen game windowed
    By rented in forum Game Programming
    Replies: 3
    Last Post: 06-11-2004, 04:19 AM
  4. make all rule
    By duffy in forum C Programming
    Replies: 9
    Last Post: 09-11-2003, 01:05 PM
  5. Replies: 6
    Last Post: 04-20-2002, 06:35 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21