Code review please

Brighteyes · 03-28-2003, 09:19 AM

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!

Codeplug · 03-28-2003, 09:58 AM

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

**Hammer** · 03-28-2003, 10:03 AM

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

**Dave_Sinkula** · 03-28-2003, 10:10 AM

Note that %[0-9] may be interpreted as equivalent to %[0123456789] as a common extension, but this is not standard.

Brighteyes · 03-28-2003, 10:27 AM

Quote:

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

Quote:

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.

Quote:

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

Quote:

> 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

Quote:

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

Quote:

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.

Quote:

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.

Quote:

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?

Codeplug · 03-28-2003, 11:07 AM

Quote:

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

Speedy5 · 03-28-2003, 03:17 PM

<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

**Hammer** · 03-28-2003, 03:20 PM

>>this is assuming you're programming in C++ since you posted in a C++ board
Errr.... what are you on about??

Brighteyes · 03-29-2003, 12:05 PM

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!

**Hammer** · 03-29-2003, 06:28 PM

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?

03-28-2003, 09:58 AM	#2
Codeplug Registered User Join Date: Mar 2003 Posts: 3,903	/* 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
Codeplug is offline

03-28-2003, 10:03 AM	#3
Hammer End Of Line Join Date: Apr 2002 Posts: 6,240	>>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]
Hammer is offline

03-28-2003, 10:10 AM	#4
Dave_Sinkula Just Lurking 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.*
Dave_Sinkula is offline

03-28-2003, 03:17 PM	#7
Speedy5 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 03:20 PM.
Speedy5 is offline

03-28-2003, 03:20 PM	#8
Hammer End Of Line Join Date: Apr 2002 Posts: 6,240	>>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]
Hammer is offline

03-29-2003, 12:05 PM	#9
Brighteyes Open to suggestions 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!
Brighteyes is offline

03-29-2003, 06:28 PM	#10
Hammer End Of Line Join Date: Apr 2002 Posts: 6,240	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]
Hammer is offline

Thread Tools
Show Printable Version Email this Page
Display Modes
Linear Mode Switch to Hybrid Mode Switch to Threaded Mode

Similar Threads
Thread	Thread Starter	Forum	Replies	Last Post
Code Review for upcoming contest	Darryl	Contests Board	2	02-28-2006 02:39 PM
Problem : Threads WILL NOT DIE!!	hanhao	C++ Programming	2	04-16-2004 01:37 PM
True ASM vs. Fake ASM ????	DavidP	A Brief History of Cprogramming.com	7	04-02-2003 04:28 AM
Interface Question	smog890	C Programming	11	06-03-2002 05:06 PM
Who will map the scan code (inserted by VKD_Force_keys) to virtual key code?	Unregistered	Windows Programming	0	02-21-2002 06:05 PM