Code review please

This is a discussion on Code review please within the C Programming forums, part of the General Programming Boards category; Hi! I'm trying my best to learn C and was wondering if anybody would tell me how I can do ...

  1. #1
    Open to suggestions Brighteyes's Avatar
    Join Date
    Mar 2003
    Posts
    204

    Smile Code review please

    Hi! I'm trying my best to learn C and was wondering if anybody would tell me how I can do better. Here's my code; a good hard critique is what I need, show no mercy.
    Code:
    #include <stdio.h>
    #include <ctype.h>
    
    int main(void)
    {
      static char line[1001]; /* static to initialize as empty */
      char name[101];
      int lastc;
      int age;
    
      while (1)
      {
        printf("Tell me your name and age: ");
        fflush(stdout);
        
        /* Get a line. Extra characters treated as junk */
        if (scanf(" %1000[^\n]%*[^\n]", line) >= 0)
          getchar();
        
        /* sscanf leaves a trailing space in 'name' if the
           name and age are separated by a space. 'lastc' marks that space */
        if (sscanf(line, "%[^0-9] %n %d", name, &lastc, &age) != 2)
          fprintf(stderr, "Unable to process. Example input: John Doe 37\n");
        else
        {
          if (isspace(name[lastc - 1]))
            name[lastc - 1] = '\0';
    
          printf("Hey, %s! You're %d years old!\n", name, age);
          break;
        }
      }
    
      return 0;
    }
    Thanks a bunch!
    p.s. What the alphabet would look like without q and r.

  2. #2
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,607
    • /* static to initialize as empty */
      You can't depend on that even though it may be true on your system. A better way to get the compiler to clear it for you is like this:
      Code:
      char line[1001] = {0};
    • Make you tabs/indents more than 2 spaces. 3 or 4 is generally more readable.
    • It's a good habbit to put all statement blocks in {} and can also add to readability. For example:
      Code:
      if (...)
      {
         getchar();
      }
    • You can also make your comment blocks a little more readable like this:
      Code:
      /* sscanf leaves a trailing space in 'name' if the
       * name and age are separated by a space. 'lastc' marks that space */


    gg

  3. #3
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    >>static char line[1001]; /* static to initialize as empty */
    To initialise as "empty", do it this way:
    >>char line[1001] = {0};


    [edit]
    Doh, beat by miles on that one Damn work getting in the way again
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  4. #4
    Just Lurking Dave_Sinkula's Avatar
    Join Date
    Oct 2002
    Posts
    5,006
    Note that %[0-9] may be interpreted as equivalent to %[0123456789] as a common extension, but this is not standard.
    7. It is easier to write an incorrect program than understand a correct one.
    40. There are two ways to write error-free programs; only the third one works.*

  5. #5
    Open to suggestions Brighteyes's Avatar
    Join Date
    Mar 2003
    Posts
    204
    > if (scanf(" %1000[^\n]%*[^\n]", line) >= 0)
    This only counts successful inputs, if the user presses ctrl-d (or ctrl-z depending on your OS), you still end up processing some data when there is none
    Yea, but since 'line' starts out with nothing in it, the error message shows up. I guess a better way would be to test for < 0 and breaking if it's true
    Code:
    /* Get a line. Extra characters treated as junk */
    if (scanf(" %1000[^\n]%*[^\n]", line) < 0)
      break;
    
    getchar();
    Would it be better to not try and process if scanf gives EOF? Or is processing an empty line and printing the error an acceptable response to that return value?
    Personally, I use fgets() to read a line to start with
    while ( fgets( buff, sizeof(buff), stdin ) != NULL ) { }

    If I care about whether there is a newline in buff or not, I do this
    char *p = strchr( buff, '\n' );
    if ( p != NULL ) { /* newline found */ }
    else { /* no newline */ }
    I tried that first, but it turns out that to get all of the stuff I wanted took up like 9 lines with fgets and made me use other functions and make new variables. The scanf way only took two lines and did everything that the fgets way did. I find it easier to read too.
    > sscanf leaves a trailing space in 'name'
    It will leave many trailing spaces, not just one if you type
    John Doe____________________42
    I can't think of a good way to fix this and still let names with spaces be typed in. The only thing I can think of is to take the age first, are there other ways?
    > if (sscanf(line, "%[^0-9]
    This is a potential buffer overflow, since you allowed the user to type in 1000 chars, and you only allowed 100 for the name
    Fixed, I just changed the size of 'name'. Thanks
    /* static to initialize as empty */
    You can't depend on that even though it may be true on your system. A better way to get the compiler to clear it for you is like this:
    Code:
    char line[1001] = {0};
    Can you prove that? Because The C Programming Language pp. 85, section 4.9 says "In the absence of explicit initialization, external and static variables are guaranteed to be initialized to zero". I still want to know even though a change to the scanf test means I don't have to initialize 'line' anymore.
    Make you tabs/indents more than 2 spaces. 3 or 4 is generally more readable.
    I don't have a problem reading 2, but I'll change it to 4 and see how that works out.
    It's a good habbit to put all statement blocks in {} and can also add to readability.
    Yea, blocks help the part with sscanf, but the others are easier to read without them in my opinion.
    Note that %[0-9] may be interpreted as equivalent to %[0123456789] as a common extension, but this is not standard.
    Thanks, I didn't know that.

    Here are the changes I've made based on your suggestions
    Code:
    #include <stdio.h>
    
    int main(void)
    {
        char line[1001];
        char name[1001];
        int age;
        
        while (1)
        {
            printf("Tell me your name and age: ");
            fflush(stdout);
            
            /* Get a line. Extra characters treated as junk */
            if (scanf("%1000[^\n]%*[^\n]", line) < 0)
                break;
            
            getchar();
            
            if (sscanf(line, "%[^0123456789] %d", name, &age) != 2)
            {
                fprintf(stderr, "Unable to process. Example input: John Doe 37\n");
            }
            else
            {
                /* 'name' may have leading and trailing spaces */
                printf("Hey, %s! You're %d years old!\n", name, age);
                break;
            }
        }
        
        return 0;
    }
    Is there anything else I can do to make it better?

  6. #6
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,607
    >> You can't depend on that even though it may be true on your system
    > Yes you can.
    Ok...learn't something new.

    Still, making a large array static just to ensure it's zero'd out isn't as good as using "={0}", I think.
    • you won't need the comment, "static to initialize as empty", since "={0}" is self-documenting (no biggy)
    • won't work if its in a function other than main() that is called more than once (assuming functionality depends on the array being zero'd on each call)


    I don't follow the {} rule completely myself

    Code is much pretty'er btw.

    gg

  7. #7
    Registered User
    Join Date
    Jan 2003
    Posts
    648
    <deleted>
    Or, if your compiler really sucks, use this to REALLY make an infinite loops without any processing (other than the jump needed to loop backwards).
    Code:
    for (;;)
    {
    }
    <deleted>

    EDIT::
    LOL, I thought I was in the C++ board so i was doing all this cout, cin, while (true) stuff. hahahah :D
    Last edited by Speedy5; 03-28-2003 at 02:20 PM.

  8. #8
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    >>this is assuming you're programming in C++ since you posted in a C++ board
    Errr.... what are you on about??
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  9. #9
    Open to suggestions Brighteyes's Avatar
    Join Date
    Mar 2003
    Posts
    204
    Thank you everyone for your wonderful comments, I still have one question that wasn't answered though.
    Code:
    if (sscanf(line, "%[^0123456789] %d", name, &age) != 2)
    As Salem said, 'name' can have a bunch of both leading and trailing spaces. What's the best way to either make sure that there aren't any to begin with, or to get rid of them if there are any. Would it be better to allow them and get rid of them before printing, or disallow the extra spaces to begin with, if that's possible with my code. I can write a trim function that gets rid of the spaces, but I'm not sure if that's the shortest/fastest/most elegant way to solve the problem.

    Thanks a bunch!

  10. #10
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    Personally, I'd probably write my own parser, that way you have more control over what goes on. Also, what happens if someones name is entered as two words?
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 01:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 01:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 03:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 05:05 PM

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