Array of structs replacing values over itself

Question

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.

You have to duplicate the strings. Because name is a char pointer to a string and all of you structs use the same pointer — hetepeperfan
– hetepeperfan, Commented Jul 13, 2013 at 21:15
like copy 'name' to another string and then store that new string into my struct? — Shamikul Amin
– Shamikul Amin, Commented Jul 13, 2013 at 21:20

Sylvain Defresne · Accepted Answer · 2013-07-15 09:14:29Z

4

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

edited Jul 15, 2013 at 9:14

answered Jul 13, 2013 at 21:20

Sylvain Defresne

45.2k12 gold badges81 silver badges88 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Shamikul Amin Over a year ago

Assignment makes pointer from integer without cast. BUT it does work though o.o

Sylvain Defresne Over a year ago

Oh, this is probably because you don't include #include <string.h>. I've updated my response. This is because, by default, if a function is not defined, C compiler assume it accept any number of arguments and returns an int.

Sylvain Defresne Over a year ago

Then, the error must be in another part of your code, because if I compile it with clang -Weverything the only warnings I get are about the size of struct student.

Shamikul Amin Over a year ago

these are the only parameters I am allowed to use with gcc: gcc -std=c89 -pedantic -o

Sylvain Defresne Over a year ago

How, this is because strdup is not part of c89 standard, but part of Posix. If you don't have access to strdup then you may have to reimplement it, I've updated my code.

Andrew · Accepted Answer · 2013-07-13 21:29:57Z

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

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.

hetepeperfan · Accepted Answer · 2013-07-13 21:33:13Z

1

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.

edited Jul 13, 2013 at 21:33

answered Jul 13, 2013 at 21:20

hetepeperfan

4,4411 gold badge34 silver badges52 bronze badges

6 Comments

0decimal0 Over a year ago

I see more than one error in his program , its basically seems a copy /paste.

Shamikul Amin Over a year ago

hmm works but complains about pointer to integer without cast

hetepeperfan Over a year ago

@PHIfounder What do you mean with 'copy paste'?

0decimal0 Over a year ago

@hetepeperfan OP must have copied the error full program and pasted it here , he should also show what output he is getting or what errors ?

hetepeperfan Over a year ago

@PHIfounder A I thought you ment I made a copy/paste. I misread your comment I suppose, sorry.

|

Collectives™ on Stack Overflow

Array of structs replacing values over itself

3 Answers 3

5 Comments

Comments

6 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

5 Comments

Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related