C For loop skips first iteration and bogus number from loop scanf
I am creating a mailing label generator for school and am having an issue with a few problems. My program is to take the full name, address, city, state, and zip code for individuals from 0 to 10. When running my program I am having two major problems. The for loop skips the full name "safergets()" and moves on to the address safergets. I moved on to see if everything else works, but my verification for the zip code would not work correctly. I added a printf to see if the input was the same number and found it to be bogus. Also, I am getting an error code for my line attempting to capitalize the state output. I'm sure I am using toupper incorrectly. Attached below is my code, error code, and output.
#include <stdio.h>
#include <ctype.h>
/* Define structure */
struct information
{
char full_name[35], address[50], city[25], state[3];
long int zip_code;
};
/* Function safer_gets */
/* ------------------- */
void safer_gets (char array[], int max_chars)
{
/* Declare variables. */
/* ------------------ */
int i;
/* Read info from input buffer, character by character, */
/* up until the maximum number of possible characters. */
/* ------------------------------------------------------ */
for (i = 0; i < max_chars; i++)
{
array[i] = getchar();
/* If "this" character is the carriage return, exit loop */
/* ----------------------------------------------------- */
if (array[i] == '\n')
break;
} /* end for */
/* If we have pulled out the most we can based on the size of array, */
/* and, if there are more chars in the input buffer, */
/* clear out the remaining chars in the buffer. */
/* ---------------------------------------------------------------- */
if (i == max_chars )
if (array[i] != '\n')
while (getchar() != '\n');
/* At this point, i is pointing to the element after the last character */
/* in the string. Terminate the string with the null terminator. */
/* -------------------------------------------------------------------- */
array[i] = '\0';
} /* end safer_gets */
/* Begin main */
int main()
{
/* Declare variables */
struct information person[10];
int x, i;
/* Issue greeting */
printf("Welcome to the mailing label generator program.\n\n");
/* Prompt user for number of individuals between 0 - 10. If invalid, re-prompt */
do
{
printf("How many people do you want to generate labels for (0-10)? ");
scanf("%i", &x);
if(x<0 || x>10)
printf("Invalid number. Please re-enter number. Must be from 0 to 10.\n");
}while(x<0 || x>10);
/* Begin loop for individual information */
for(i = 0; i < x; i++)
{
printf("\n\nEnter name: ");
safer_gets(person[i].full_name, 35); /* This is the step being skipped */
printf("\nEnter street address: ");
safer_gets(person[i].address, 50);
printf("\nEnter city: ");
safer_gets(person[i].city, 25);
printf("\nEnter state: ");
gets(person[i].state);
/* Begin loop to verify correct zipcode */
do
{
printf("\nEnter zipcode: ");
scanf("%ld", person[i].zip_code); /* I get a bogus number here */
if(person[i].zip_code<00001 || person[i].zip_code>99999)
{
printf("\nInvalid zipcode. Must be from 00001 to 99999.");
}
}while(person[i].zip_code<00001 || person[i].zip_code>99999);
/* end loop */
}/* end of loop */
/* Output individual information in mailing format, condition for 0 individuals */
if(x>0 && x<10)
{
printf("\n\nBelow are your mailing labels:\n\n");
}
/* Begin loop for outputting individual(s) mailing labels */
for(i = 0; i < x; i++)
{
printf("%s\n",person[i].full_name);
printf("%s\n",person[i].address);
printf("%s\n",person[i].city);
/* Output state in all uppercase */
printf("%s\n", toupper(person[i].state)); /* This is where the error code is occurring */
printf("%.5ld\n\n", person[i].zip_code);
} /* end of loop */
printf("Thank you for using the program.\n");
}/*end of main */
Error code:142: warning: passing arg 1 of `toupper' makes integer from pointer without a cast.
Output:
Welcome to the mailing label generator program.
How many people do you want to generate labels for (0-10)? 1
Enter name:
Enter street address: 100 Needhelp Ave.
Enter city: Gardner
Enter state: NY
Enter zipcode: 01420
Invalid zipcode. Must be from 00001 to 99999.
Enter zipcode:
I have looked at several questions on here to try and understand where I am going wrong, but if feel it might be a couple of issues effecting my program. Also, the safergets function was given to my class by our professor to ensure that the user does not input more characters than the array can hold. Thank you for your help and tolerance in helping me understand my mistakes!
Solution 1:
Let's take a look at the problem one by one:
newline Remains in stdin after No. or Persons Read
printf("\n\nEnter name: "); safer_gets(person[i].full_name, 35); /* This is the step being skipped */
It is skipped because your safer_gets() reads only to the first '\n' (newline character -- not a carriage-return, that is '\r'). However, the first character saver_gets() sees in the input stream is the '\n' character that remains in stdin unread after your call to scanf in:
printf("How many people do you want to generate labels for (0-10)? "); scanf("%i", &x);
All scanf format specifiers for numeric conversion read only through the last digit (or decimal point) that makes up a number leaving the '\n' generated by the user pressing Enter unread in the input-stream (stdin here). This is one of the primary reasons new C programmers are encouraged to read user input with a line-oriented input function such as fgets() (or POSIX getline()) and then use sscanf to parse values from the filled buffer.
Why Line-Oriented Input Functions are Preferred for User-Input
By using a line-oriented input function with sufficient buffer, the complete line of user-input is consumed (including the '\n' from the user pressing Enter). This ensures that stdin is ready for the next input and doesn't have unread characters left-over from a prior input waiting to bite you.
Using All Input Functions Correctly
If you take nothing else from this answer, learn this -- you cannot use any input function correctly unless you check the return. This is especially true for the scanf family of functions. Why? If you are attempting to read an integer with scanf and the user enters "four" instead, then a matching failure occurs and character extraction from your input stream ceases with the first invalid character leaving all offending characters in your input stream unread. (just waiting to bite you again).
Using scanf Correctly
scanf can be used, if used correctly. This means you are responsible for checking the return of scanf every time. You must handle three conditions
-
(return == EOF)the user canceled input by generating a manualEOFby pressing Ctrl+d (or on windows Ctrl+z); -
(return < expected No. of conversions)a matching or input failure occurred. For a matching failure you must account for every character left in your input buffer. (scan forward in the input buffer reading and discarding characters until a'\n'orEOFis found); and finally -
(return == expected No. of conversions)indicating a successful read -- it is then up to you to check whether the input meets any additional criteria (e.g. positive integer, positive floating-point, within a needed range, etc..).
You also must account for what is left in your input stream after a successful read with scanf. As discussed above, scanf will leave the '\n' in the input stream unread for ALL conversion specifiers unless you specifically account for it in your format string (which, if accounted for, usually results in a fragile input format string, easily foiled by additional extraneous characters after the wanted input but before the '\n') When using scanf for input, you must put your accountant's hat on and account for every character that remains in the input stream and empty the input stream of any offending characters when required.
You can write a simple empty_stdin() function to handle removing all extraneous characters that remain after a user input by simply scanning forward discarding all characters that remain until the '\n' is found or EOF encountered. You do that to a varying degree in your safer_gets() function. You can write a simple function as:
void empty_stdin(void)
{
int c = getchar(); /* read character */
while (c != '\n' && c != EOF) /* if not '\n' and not EOF */
c = getchar(); /* repeat */
}
You can do the same thing with a simple for loop inline, e.g.
for (int c = getchar(); c != '\n' && c != EOF; c = getchar()) {}
Next Problem -- Attempting to Write to Invalid Address
When using scanf, scanf expects the parameter for a corresponding conversion to be a pointer to the appropriate type. In:
printf("\nEnter zipcode: "); scanf("%ld", person[i].zip_code); /* I get a bogus number here */
You fail to provide a pointer, providing a long int value instead. Since person[i].zip_code is type long int in order to provide a pointer for scanf to fill you must use the address-of operator, e.g. &person[i].zip_code to tell scanf which address to fill with the value it provides a conversion for.
Wait? Why don't I have to do that with array? On access, an array is converted to a pointer to the first element. So for string input, if an array is being used to hold the string, it is automatically converted to a pointer C11 Standard - 6.3.2.1 Other Operands - Lvalues, arrays, and function designators(p3).
toupper Operates on Characters not Strings
printf("%s\n", toupper(person[i].state)); /* This is where the error code is occurring */
As discussed in my comment, toupper takes type int as the parameter, not type char*. To convert a string to upper/lower case, you need to loop over each character converting each character individually. However, in your case with the .state member of your struct, there are only 2 characters to worry about, so just convert them both when they are read, e.g.
/* just 2-chars to convert to upper - do it here */
person[i].state[0] = toupper (person[i].state[0]);
person[i].state[1] = toupper (person[i].state[1]);
Fundamental Problems in safer_gets()
That takes care of the most of the obvious problems, but the safer_gets() function itself has several fundamental problems. Specifically, it fails to handle EOF when returned by getchar() and it fails to provide any indication to the user whether the requested user-input succeeded or failed due to returning nothing with type void. In any function you write, where there is any possibility of failure within the function, you MUST provide a meaningful return to the calling function to indicate whether the requested operation of the function succeeded or failed.
What can you do with safer_gets()? Why not return a simple int value providing the number of characters read on success, or -1 (the normal value for EOF) on failure. You get the double-bonus of now being able to validate whether the input succeeded -- and you also get the number of character in the string (limited to 2147483647 chars). You also now have the ability to handle a user canceling the input by generating a manual EOF with Ctrl+d on Linux or Ctrl+z (windows).
You should also empty stdin of all characters input in ALL cases other than EOF. This ensures there are no characters that remain unread after your call to safer_gets() that can bite you if you make a call to another input function later. Making those changes, you could write your safer_gets() as:
/* always provide a meaninful return to indicate success/failure */
int safer_gets (char *array, int max_chars)
{
int c = 0, nchar = 0;
/* loop while room in array and char read isn't '\n' or EOF */
while (nchar + 1 < max_chars && (c = getchar()) != '\n' && c != EOF)
array[nchar++] = c; /* assing to array, increment index */
array[nchar] = 0; /* nul-terminate array on loop exit */
while (c != EOF && c != '\n') /* read/discard until newline or EOF */
c = getchar();
/* if c == EOF and no chars read, return -1, otherwise no. of chars */
return c == EOF && !nchar ? -1 : nchar;
}
(note: above the test on nchar + 1 < max_chars ensures that a character remains for the nul-terminating character, and is just a safer rearrangement of nchar < max_chars - 1)
General Approach to Input Validation
Now, you have an input function you can use that indicates success/failure of input allowing you to validate the input back in the calling function (main() here). Take for example reading the .full_name member using safer_gets(). You can't just blindly call safer_gets() and not know whether input was canceled or a premature EOF encountered and use then proceed to use the string it filled with any confidence in your code. *Validate, validate, validate each expression. Back in main(), you can do that by calling safer_gets() as follows to read .full_name (and every other string variable):
#define NAMELEN 35 /* if you need a constant, #define one (or more) */
#define ADDRLEN 50 /* (don't skimp on buffer size) */
...
for (;;) { /* loop continually until valid name input */
fputs ("\nEnter name : ", stdout); /* prompt */
int rtn = safer_gets(person[i].full_name, NAMELEN); /* read name */
if (rtn == -1) { /* user canceled input */
puts ("(user canceled input)");
return 1; /* handle with graceful exit */
}
else if (rtn == 0) { /* if name empty - handle error */
fputs (" error: full_name empty.\n", stderr);
continue; /* try again */
}
else /* good input */
break;
}
(note: the return of safer_gets() is captured in the variable rtn and then evaluated for -1 (EOF), 0 empty-string, or greater than 0, good input)
You can do that for each string variable you need to use, and then use the same principals discussed above to read and validate .zip_code. Putting it altogether in a short example, you could do:
#include <stdio.h>
#include <ctype.h>
#define NAMELEN 35 /* if you need a constant, #define one (or more) */
#define ADDRLEN 50 /* (don't skimp on buffer size) */
#define CITYLEN 25
#define STATELEN 3
#define PERSONS 10
struct information {
char full_name[NAMELEN],
address[ADDRLEN],
city[CITYLEN],
state[STATELEN];
long int zip_code;
};
/* always provide a meaninful return to indicate success/failure */
int safer_gets (char *array, int max_chars)
{
int c = 0, nchar = 0;
/* loop while room in array and char read isn't '\n' or EOF */
while (nchar + 1 < max_chars && (c = getchar()) != '\n' && c != EOF)
array[nchar++] = c; /* assing to array, increment index */
array[nchar] = 0; /* nul-terminate array on loop exit */
while (c != EOF && c != '\n') /* read/discard until newline or EOF */
c = getchar();
/* if c == EOF and no chars read, return -1, otherwise no. of chars */
return c == EOF && !nchar ? -1 : nchar;
}
int main (void) {
/* declare varaibles, initialize to all zero */
struct information person[PERSONS] = {{ .full_name = "" }};
int i = 0, x = 0;
puts ("\nWelcome to the mailing label generator program.\n"); /* greeting */
for (;;) { /* loop continually until a valid no. of people entered */
int rtn = 0; /* variable to hold RETURN from scanf */
fputs ("Number of people to generate labels for? (0-10): ", stdout);
rtn = scanf ("%d", &x);
if (rtn == EOF) { /* user generated manual EOF (ctrl+d [ctrl+z windows]) */
puts ("(user canceled input)");
return 0;
}
else { /* either good input or (matching failure or out-of-range) */
/* all required clearing though newline - do that here */
for (int c = getchar(); c != '\n' && c != EOF; c = getchar()) {}
if (rtn == 1) { /* return equals requested conversions - good input */
if (0 <= x && x <= PERSONS) /* validate input in range */
break; /* all checks passed, break read loop */
else /* otherwise, input out of range */
fprintf (stderr, " error: %d, not in range 0 - %d.\n",
x, PERSONS);
}
else /* matching failure */
fputs (" error: invalid integer input.\n", stderr);
}
}
if (!x) { /* since zero is a valid input, check here, exit if zero requested */
fputs ("\nzero persons requested - nothing further to do.\n", stdout);
return 0;
}
/* Begin loop for individual information */
for (i = 0; i < x; i++) { /* loop until all person filled */
/* read name, address, city, state */
for (;;) { /* loop continually until valid name input */
fputs ("\nEnter name : ", stdout); /* prompt */
int rtn = safer_gets(person[i].full_name, NAMELEN); /* read name */
if (rtn == -1) { /* user canceled input */
puts ("(user canceled input)");
return 1; /* handle with graceful exit */
}
else if (rtn == 0) { /* if name empty - handle error */
fputs (" error: full_name empty.\n", stderr);
continue; /* try again */
}
else /* good input */
break;
}
for (;;) { /* loop continually until valid street input */
fputs ("Enter street address : ", stdout); /* prompt */
int rtn = safer_gets(person[i].address, ADDRLEN); /* read address */
if (rtn == -1) { /* user canceled input */
puts ("(user canceled input)");
return 1; /* handle with graceful exit */
}
else if (rtn == 0) { /* if address empty - handle error */
fputs ("error: street address empty.\n", stderr);
continue; /* try again */
}
else /* good input */
break;
}
for (;;) { /* loop continually until valid city input */
fputs ("Enter city : ", stdout); /* prompt */
int rtn = safer_gets(person[i].city, CITYLEN); /* read city */
if (rtn == -1) { /* user canceled input */
puts ("(user canceled input)");
return 1; /* handle with graceful exit */
}
else if (rtn == 0) { /* if city empty - handle error */
fputs ("error: city empty.\n", stderr);
continue; /* try again */
}
else /* good input */
break;
}
for (;;) { /* loop continually until valid state input */
fputs ("Enter state : ", stdout); /* prompt */
int rtn = safer_gets(person[i].state, STATELEN); /* read state */
if (rtn == -1) { /* user canceled input */
puts ("(user canceled input)");
return 1; /* handle with graceful exit */
}
else if (rtn == 0) { /* if state empty - handle error */
fputs ("error: state empty.\n", stderr);
continue; /* try again */
}
else { /* good input */
/* just 2-chars to convert to upper - do it here */
person[i].state[0] = toupper (person[i].state[0]);
person[i].state[1] = toupper (person[i].state[1]);
break;
}
}
/* read/validate zipcode */
for (;;) { /* loop continually until valid zipcode input */
fputs ("Enter zipcode : ", stdout); /* prompt */
int rtn = scanf ("%ld", &person[i].zip_code); /* read zip */
if (rtn == EOF) { /* user pressed ctrl+d [ctrl+z windows] */
puts ("(user canceled input)");
return 1;
}
else { /* handle all other cases */
/* remove all chars through newline or EOF */
for (int c = getchar(); c != '\n' && c != EOF; c = getchar()) {}
if (rtn == 1) { /* long int read */
/* validate in range */
if (1 <= person[i].zip_code && person[i].zip_code <= 99999)
break;
else
fprintf (stderr, " error: %ld not in range of 1 - 99999.\n",
person[i].zip_code);
}
else /* matching failure */
fputs (" error: invalid long integer input.\n", stderr);
}
}
}
/* Output individual information in mailing format, condition for 0 individuals */
for(i = 0; i < x; i++)
/* you only need a single printf */
printf ("\n%s\n%s\n%s, %s %ld\n", person[i].full_name, person[i].address,
person[i].city, person[i].state, person[i].zip_code);
fputs ("\nThank you for using the program.\n", stdout);
}
(note: by using #define to create the needed constants, if you need to adjust a number, you have a single place to make the change and you are not left picking though each variable declaration and loop limit to try and make the change)
Example Use/Output
When you have finished writing any input routine -- go try and break it! Find the corner-cases that fail and fix them. Keep trying to break it by intentionally entering incorrect/invalid input until it no longer excepts anything but what the user is required to input. Exercise your input routines, e.g.
$ ./bin/nameaddrstruct
Welcome to the mailing label generator program.
Number of people to generate labels for? (0-10): 3
Enter name : Mickey Mouse
Enter street address : 111 Disney Ln.
Enter city : Orlando
Enter state : fL
Enter zipcode : 44441
Enter name : Minnie Mouse
Enter street address : 112 Disney Ln.
Enter city : Orlando
Enter state : Fl
Enter zipcode : 44441
Enter name : Pluto (the dog)
Enter street address : 111-b.yard Disney Ln.
Enter city : Orlando
Enter state : fl
Enter zipcode : 44441
Mickey Mouse
111 Disney Ln.
Orlando, FL 44441
Minnie Mouse
112 Disney Ln.
Orlando, FL 44441
Pluto (the dog)
111-b.yard Disney Ln.
Orlando, FL 44441
Thank you for using the program.
Respecting the users wish to cancel input at any point when they generates a manual EOF with Ctrl+d on Linux or Ctrl+z (windows), you should be able to handle that from any point in your code.
At the first prompt:
$ ./bin/nameaddrstruct
Welcome to the mailing label generator program.
Number of people to generate labels for? (0-10): (user canceled input)
Or at any prompt thereafter:
$ ./bin/nameaddrstruct
Welcome to the mailing label generator program.
Number of people to generate labels for? (0-10): 3
Enter name : Mickey Mouse
Enter street address : 111 Disney Ln.
Enter city : (user canceled input)
Handle a request for zero person:
$ ./bin/nameaddrstruct
Welcome to the mailing label generator program.
Number of people to generate labels for? (0-10): 0
zero persons requested - nothing further to do.
(**personally, I would just change the input test and have them enter a value from 1-10 instead)
Invalid input:
$ ./bin/nameaddrstruct
Welcome to the mailing label generator program.
Number of people to generate labels for? (0-10): -1
error: -1, not in range 0 - 10.
Number of people to generate labels for? (0-10): 11
error: 11, not in range 0 - 10.
Number of people to generate labels for? (0-10): banannas
error: invalid integer input.
Number of people to generate labels for? (0-10): 10
Enter name : (user canceled input)
You get the point... Bottom line, you must validate every user input and know it is valid before making use of the input in your program. You cannot validate any input from any function unless you check the return. If you take away nothing except that, the learning has been worthwhile.
Look things over and let me know if you have further questions. (and ask your prof. how safer_gets() handles EOF and how you are supposed to validate whether the funciton succeeded or failed)