Array of structs replacing values over itself

Ok so I have the below code and I am just pulling various things from a file and inputing them in an array of structs, it "seemingly" works initially, BUT when I go to printing it after it is done with the file it seemed to have replaced all of the courses and names with the very last vale, oddly this doesnt happen with the integers (grades), the grades do get inputed properly.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct student {
    char *name;
    char *course;
    int grade;

};

void courseSort(struct student d[20], int size);

int main(void)
{
    FILE* fp;
    char* filename = "grades.csv";
    char buffer[100];
    char* name, *class;
    char* del=",";
    int grade, i, counter=0;

    struct student d[20];

    if((fp=fopen(filename, "r"))==NULL)
    {
        printf("unable to open %s\n", filename);
        exit(1);
    }

    while(fgets(buffer, sizeof(buffer), fp) !=NULL)
    {
        name = strtok(buffer,del);
        class=strtok(NULL,del);
        grade = atoi(strtok(NULL,del));

        d[counter].name=name;
        d[counter].course=class;
        d[counter].grade=grade;
        printf("%s    %s       %d\n",d[counter].name,d[counter].course,d[counter].grade);
        counter++;
    }

    printf("\n\n\n");

    for(i=0;i<counter;i++)
        printf("%s    %s     %d\n",d[i].name,d[i].course,d[i].grade);
    courseSort(d,counter);

    fclose(fp);
 }

I am not sure what I am doing wrong :( it all seems straightforward but not sure why it just replaces everything with the latest one.

The strtok returns a pointer to the buffer and does not allocate memory. Since you do not copy the strings, you end up with lots of pointers pointing to the same buffer that is overwritten at each iteration of the loop.

To fix this, you need to change your loop to copy the strings using strdup:

while(fgets(buffer, sizeof(buffer), fp) != NULL)
{
    d[counter].name = strdup(strtok(buffer, del));
    d[counter].course = strdup(strtok(NULL, del));
    d[counter].grade = atoi(strtok(NULL, del));
    counter++;
}

Don't forget to return the allocated memory with free once you no longer need the strings:

for (i = 0; i < counter; i++) {
   free(d[i].name);
   free(d[i].course);

   d[i].name = NULL;
   d[i].course = NULL;
}

Note that strdup is part of POSIX1.2001 standard, not part of C89. If it is not available, you'll have to re-implement it yourself (quite easy):

char *my_strdup(const char *str) {
  char *copy;
  size_t len = strlen(str) + 1;
  if (len == 0) return NULL;
  copy = (char *)malloc(len);
  if (copy == NULL) return NULL;
  memcpy(copy, str, len);
  return copy;
}

This is a simple misunderstanding of pointers and char arrays (strings). Here are a couple pages that explains them pretty well:

http://www.cplusplus.com/doc/tutorial/pointers/
http://www.cplusplus.com/doc/tutorial/ntcs/

In your case, you are setting your struct pointer values equal to the returned pointer from strtok. A lot of those string functions work by putting the result at a certain memory address and returning the pointer to it. The pointer returned is always the same, so all your struct values are going to point to the last result of the strtok call.

This is why you need strdup (String duplicate). Basically it takes the value at the address given and copies the contents into a new place in memory and returns the value.

The error is here.

d[counter].name=name;

replace with:

d[counter].name = strdup(name); /*don't forget to free this memory.*/

the issue for the courses is the same.

Array of structs replacing values over itself

Related

Recent Posts