Continuing from my comments, you have a large number of small problems.
First, always compile with FULL warnings enabled, and do not accept code until it compiles without warning. To enable warnings add -Wall -Wextra -pedantic to your gcc/clang compile string (also consider adding -Wshadow to warn on shadowed variables). For VS (cl.exe on windows), use /W3. All other compilers will have similar options. Read and understand each warning -- then go fix it.
That would first disclose missing parenthesis for:
if ((fp = fopen(argv[1], "r")) == NULL) /* parenthesis req'd */
It would have shown your next error to be with your declaration for delim. strtok() takes a character string as the delimiter list (and it is constant, so correctly):
const char *delim = ",\n"; /* don't forget \n */
(note: include '\n' or you will leave a '\n' hanging off your last token)
There is no +1 with the size needed by fgets(). If you use the sizeof the array you are filling, you can't go wrong, e.g.
while (fgets (buffer, sizeof buffer, fp))
Now we begin to reach your real problems with strtok() and storage for each name. char *name; in each struct is a uninitialized pointer that points nowhere. While you can assign a pointer to it, you don't want to simply assign the same pointer address to all of the .name members or they will all end up pointing to the same place.
Instead, you need to allocate storage for each name and copy the current name to the allocated block of memory preserving that name., e.g.
data_t customer;
char *tmp; /* temporary pointer to use w/strtok() */
size_t len;
tmp = strtok (buffer, delim); /* token for name */
if (!tmp) { /* validate token */
fputs ("error: strtok() name.\n", stderr);
return 1;
}
len = strlen (tmp); /* get length */
customer.name = malloc (len + 1); /* allocate for string */
if (!customer.name) { /* validate EVERY allocation */
perror ("malloc-customer.name");
return 1;
}
memcpy (customer.name, tmp, len + 1); /* copy tmp to .name */
(note: there is no need to scan for the end-of-string again with strcpy() when you already know the number of characters to copy, just use memcpy() for that)
Again, you have the "assigning the same pointer" problem with array[index]. You want to copy the contents of data_t customer; to array[index]. You allocate storage for the new struct (good job), but then you overwrite the address for the allocated block with the address of &customer creating a memory leak.
Assign the struct, not it's address. The members will be copied to the allocated struct which is what you want, e.g.
array[index] = malloc (sizeof *array[index]); /* allocate struct */
*array[index] = customer; /* assign struct */
index += 1;
(note: the allocation with malloc() uses the dereferenced pointer (e.g. *array[index]) to set the type-size. If you always use the dereferenced pointer instead of a type -- you will never get the type-size wrong. Also, in C, there is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc? )
Now things should work a little better. Don't forget to free (array[i].name) before you free (array[i]).
Also, as mentioned above, if you have POSIX strdup() you can allocate and copy in a single operation. But note, strdup() isn't standard C, so using it may result in portability problems. However, if you do, you can simply:
/* if you have POSIX strdup(), you can simply allocate and copy
* in one operation
*/
strdup (customer.name, tmp);
But...
/* since strdup() allocates, you must validate */
if (!customer.name) {
perror ("strdup-customer.name");
return 1;
}
It saves a line or two of code, but at the price of standard-C portability. (up to you)
Lastly, avoid atoi() and atof() they provide ZERO error checking and will happily accept atoi ("my cow") silently returning 0 without any indication an error occurred. Use strtol() and strtod() instead, or at least sscanf() where you at least get a success or failure indication for the conversion.
Full Example
The other issues that linger are your use of MagicNumbers. Don't use them, instead:
#define NELEM 10 /* if you need a constant, #define one (or more) */
#define MAXB 128
You also need to protect your allocated block of pointers so you don't attempt to access pointers that are not allocated. You do that in your read-loop, e.g.
/* protect the allocation bounds, you only have NELEM pointers */
while (index < NELEM && fgets (buffer, sizeof buffer, fp))
A quick revision with all validations included would look something like:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define NELEM 10 /* if you need a constant, #define one (or more) */
#define MAXB 128
typedef struct data data_t;
struct data
{
char *name;
int id;
double balance;
};
int main(int argc, char **argv)
{
FILE *fp = NULL;
data_t **array = malloc (NELEM * sizeof *array);
if (!array) { /* validate EVERY allocation */
perror ("malloc-array");
return 1;
}
if (argc < 2) {
printf("No file input\n");
exit(1);
}
if ((fp = fopen(argv[1], "r")) == NULL) { /* parenthesis req'd */
printf("File not found\n");
exit(1);
}
char buffer[MAXB];
const char *delim = ",\n"; /* don't forget \n */
int index = 0;
/* protect the allocation bounds, you only have NELEM pointers */
while (index < NELEM && fgets (buffer, sizeof buffer, fp))
{
data_t customer;
char *tmp; /* temporary pointer to use w/strtok() */
size_t len;
tmp = strtok (buffer, delim); /* token for name */
if (!tmp) { /* validate token */
fputs ("error: strtok() name.\n", stderr);
return 1;
}
len = strlen (tmp); /* get length */
customer.name = malloc (len + 1); /* allocate for string */
if (!customer.name) { /* validate EVERY allocation */
perror ("malloc-customer.name");
return 1;
}
memcpy (customer.name, tmp, len + 1); /* copy tmp to .name */
if (!(tmp = strtok(NULL, delim))) { /* token & validations */
fputs ("error: strtok() - id.\n", stderr);
return 1;
}
/* MINIMAL conversion validation with sscanf() */
if (sscanf (tmp, "%d", &customer.id) != 1) {
fputs ("error: invalid integer value - id.\n", stderr);
return 1;
}
if (!(tmp = strtok(NULL, delim))) { /* token & validations */
fputs ("error: strtok() - balance.\n", stderr);
return 1;
}
if (sscanf (tmp, "%lf", &customer.balance) != 1) { /* dito */
fputs ("error: invalid integer value - balance.\n", stderr);
return 1;
}
array[index] = malloc (sizeof *array[index]); /* allocate struct */
if (!array[index]) { /* validate!! */
perror ("malloc-array[index]");
return 1;
}
*array[index] = customer; /* assign struct */
index += 1;
}
fclose(fp);
fp = NULL; /* not needed, but doesn't hurt anything */
/* use array[], then don't forget to free memory */
for (int i = 0; i < index; i++) {
printf ("\nname : %s\n"
"id : %d\n"
"balance : %.2f\n",
array[i]->name, array[i]->id, array[i]->balance);
free (array[i]->name); /* free allocated block for name */
free (array[i]); /* free allocated array index */
}
free (array); /* don't forget to free pointers */
}
(note: you don't use assert(), so there is no need to include the header)
Since strtok() can return NULL on failure, you should assign all pointers to tmp and then validate if (!tmp) { fputs ("error: strtok() - name.\n", stderr); return 1; } and the same for ID and balance before using the pointer in the numeric conversion. You should at minimum use a simple sscanf() validation for each numeric conversion as shown above. Ideally, you would use strtol() and strtod() which provides much more information on error.
Good luck with your coding!
