Thread: Code review please

• /* 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

> 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?

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

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!