In addition to not writing beyond the end of your array-of-structs when reading your data, there are several additional areas where you may want to revise the approach you have taken with your code.
First, unless there is a compelling reason to declare your data structure as a global variable, you should limit its scope to main() and pass the array-of-structs as a parameter to any functions that need access to the data. In addition, when dealing with constants in your program (e.g. max phonebook entries 1000), it is good practice to either define a constant (#define MAXE 1000) or preferably use an enum to define the constant, e.g.:
enum { MAXE = 1000 };
(An anonymous enum is fine.)
You can also simplify your life by creating a typedef to your struct as well which will make passing the array-of-structs as a parameter easier. For instance you can declare a typedef to your struct (either named or anonymous) as follows:
typedef struct {
char Firstname[25];
char Lastname[25];
char street[20];
int street_no ;
char city[15];
int number;
char email[50];
} pb;
This will allow a simple declaration in main(), e.g.:
pb k[MAXE] = {{{0},{0},{0},0,{0},0,{0}}};
While not required, it is also good practice to initialize all variables, (including your array-of-structs), when they are declared.
While in this instance, there is little difference between reading the data file with fscanf or using a line-oriented input function, you will generally find reading a line at a time with fgets or getline and then parsing the line into components with sscanf or simple pointers will provide a more flexible and robust input routine. Regardless whether you read with fscanf or read with fgets and parse with sscanf always check the returns of fscanf or sscanf to validate the number of successful conversions.
The benefit of using line-oriented input functions to read each line of text from your input file is it will eliminate the rigidity of the fscanf format-string from the actual read of the file and allow you to handle separating the values after the line has been successfully read into a buffer. An example of using fgets in your case could be:
/* read addresses from input file up to a maximum of MAXE
* addresses. updates 'idx' pointer to hold the number of
* addreses read from file and returns number read
*/
size_t read_str (pb (*k)[], size_t *idx, FILE *fp)
{
char tmp[MAXL] = {0};
while (*idx < MAXE && fgets (tmp, MAXL, fp)) {
// printf ("read[%zu]\n", *idx);
if (sscanf (tmp, " %24[^,],%24[^,],%19[^,],%d,%14[^,],%d,%49[^\n]",
(*k)[*idx].Firstname, (*k)[*idx].Lastname,
(*k)[*idx].street, &(*k)[*idx].street_no,
(*k)[*idx].city, &(*k)[*idx].number, (*k)[*idx].email) != 7) {
fprintf (stderr, "read_str() error: parse of line[%zu] failed.\n",
*idx);
break;
}
(*idx)++;
}
return *idx;
}
note also the return of the number of address entries read allowing you to gauge success/failure of the function as well as providing you with the number of entries read. The number of entries read (idx) is also passed as a pointer to the function making the number of entries available in the calling function (main() here) regardless of whether the return is assigned.
Beyond those initial issues, you will want to validate each action you take that has consequence for the continued operation of your code. (e.g. all file opens, reads, writes, etc...) Putting those pieces together and adding basic validation, and using line oriented input, another approach to your task could look like the following:
#include <stdio.h>
/* constants for max input line and max entries */
enum { MAXL = 256, MAXE = 1000 };
typedef struct {
char Firstname[25];
char Lastname[25];
char street[20];
int street_no ;
char city[15];
int number;
char email[50];
} pb;
size_t read_str (pb (*k)[], size_t *idx, FILE *fp);
void print_str_fmt (pb *k, size_t idx);
int print_str (pb *k, size_t idx, FILE *fp);
int main (int argc, char **argv) {
if (argc < 3) { /* validate input/output filenames given as arguments */
fprintf (stderr, "error: insufficient input, usage: %s infile outfile\n",
argv[0]);
return 1;
}
pb k[MAXE] = {{{0},{0},{0},0,{0},0,{0}}}; /* initialize variables */
size_t index = 0;
FILE *ifp, *ofp;
if (!(ifp = fopen (argv[1], "r"))) { /* validate input file open */
fprintf (stderr, "error: file open failed '%s'\n", argv[1]);
return 1;
}
if (!(ofp = fopen (argv[2], "w"))) { /* validate output file open */
fprintf (stderr, "error: file open failed '%s'\n", argv[2]);
return 1;
}
if (!read_str (&k, &index, ifp)) { /* validate entries read */
fprintf (stderr, "error: read_str - no addresses read\n");
return 1;
}
fclose (ifp); /* close input file */
printf ("The addresses are:\n\n");
print_str_fmt (k, index);
if (print_str (k, index, ofp)) { /* validate entries written */
fprintf (stderr, "error: print_str - no addresses read\n");
return 1;
}
fclose (ofp); /* close output file */
return 0;
}
/* read addresses from input file up to a maximum of MAXE
* addresses. updates 'idx' pointer to hold the number of
* addreses read from file and returns number read
*/
size_t read_str (pb (*k)[], size_t *idx, FILE *fp)
{
char tmp[MAXL] = {0};
while (*idx < MAXE && fgets (tmp, MAXL, fp)) {
// printf ("read[%zu]\n", *idx);
if (sscanf (tmp, " %24[^,],%24[^,],%19[^,],%d,%14[^,],%d,%49[^\n]",
(*k)[*idx].Firstname, (*k)[*idx].Lastname,
(*k)[*idx].street, &(*k)[*idx].street_no,
(*k)[*idx].city, &(*k)[*idx].number, (*k)[*idx].email) != 7) {
fprintf (stderr, "read_str() error: parse of line[%zu] failed.\n",
*idx);
break;
}
(*idx)++;
}
return *idx;
}
/* formatted print of addressbook to stdout */
void print_str_fmt (pb *k, size_t idx)
{
size_t i;
for (i = 0; i < idx; i++)
printf (" %s %s\n %s No. %d\n %s, %d\n %s\n\n",
k[i].Firstname, k[i].Lastname, k[i].street, k[i].street_no,
k[i].city, k[i].number, k[i].email);
}
int print_str (pb *k, size_t idx, FILE *fp)
{
size_t i;
for (i = 0; i < idx; i++)
if (fprintf (fp, "%s,%s,%s,%d,%s,%d,%s\n",
k[i].Firstname, k[i].Lastname, k[i].street, k[i].street_no,
k[i].city, k[i].number, k[i].email) < 0)
return 1;
return 0;
}
Compile
gcc -Wall -Wextra -O3 -o bin/readstructsscanf readstructsscanf.c
Test Input
$ cat ../dat/phonebook.txt
Ahmed,Mohamed,26 Elhoreya Street,15,Alexandria,4876321,ahmed@gmail.com
Sarah,Zaki,7 Smouha,36,Alexandria,3974542,sarah@hotmail.com
Use/Output
$ ./bin/readstructsscanf ../dat/phonebook.txt foo.txt
The addresses are:
Ahmed, Mohamed
26 Elhoreya Street No. 15
Alexandria, 4876321
ahmed@gmail.com
Sarah, Zaki
7 Smouha No. 36
Alexandria, 3974542
sarah@hotmail.com
Confirm Output File
$ diff ../dat/phonebook.txt foo.txt
$
As with all problems in C, there are usually many ways to approach a correct solution. Hopefully this will give you a few additional ideas on how to make your code more flexible and robust.
