Copy the file content to a struct

Question

I try to take the name and the code of students from a file and save it to a structure. The file look like this

6, student1
2, student2
9, student3

and here is my attempt. Can you tell me what is wrong in this code?

#include <stdio.h>
#include <stdlib.h>
#define N 20

struct Students {
    char studentName[N];
    int code;
};

void student() {
    FILE *f = fopen("file.txt", "r");
    struct Students *ptr;
    int i;
    ptr = malloc(sizeof(struct Students));
    if (ptr == NULL){
        printf("memory cannot be allocated");
        exit(1);
    }
    if (f == NULL) {
        printf("can't open the file");
        exit(1);
    }
    i=0;
    while (feof(f)) {
        fscanf(f, "%d, %s\n", &(ptr+i)->code, &(ptr+i)->studentName);    
        ptr = realloc(ptr, sizeof(struct Students) * i);
        i++;
    }
}

int main() {
    student();
    return 0;
}

`while(feof(f))` is certainly not right. You probably meant `while(!feof(f))` but that's wrong too, see https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong — Nate Eldredge, Jul 01 '20 at 19:04
Also, your `realloc` logic is wrong. Think carefully through it: at each step, how many structs can `ptr` hold, what's the index of the last one, and what index are you writing to? — Nate Eldredge, Jul 01 '20 at 19:09
In adition `fscanf(f, "%d, %s\n", &(ptr+i)->code, (ptr+i)->studentName);`, `&(ptr+i)->studentName` should be `(ptr+i)->studentName` or better yet `ptr[i].studentName` — anastaciu, Jul 01 '20 at 19:13
in your first realloc you are multiplying with i = 0. It surely not what you want — guyr79, Jul 01 '20 at 19:58

roozbeh sharifnasab · Accepted Answer · 2021-03-28T21:30:09.473

i debugged your code, there was few problems that we will see them:

#include <stdio.h>
#include <stdlib.h>
#define N 20

struct Student { // 1- changed name from Students to Student
    char name[N];
    int code;
};

struct Student* student() {
    FILE *f = fopen("file.txt", "r");
    if (f == NULL) {
        printf("can't open the file");
        exit(1);
    }


    struct Student *ptr = NULL; // 2- initialize pointer with null

    int i;
    for(i=0; !feof(f); i++) { // 3- replace while with for and changed condition to !foef(f)
        ptr = realloc(ptr, sizeof(struct Student) * (i+1)); // 4- allocate with size (i+1) because i is always size-1 and less than we need.
        fscanf(f, "%d, %s\n", &ptr[i].code, ptr[i].name); // 5- changed syntax of accessing fields of structs
    }
    return ptr; // 6- return pointer to free it later and prevent memory leak
}

int main() {
    struct Student* ptr = student();
    // use it and free it
    return 0;
}

explain:

your struct Student contains information of only one student not many students so it is better to name it Student. I also renamed studentCode to code because it is obvious that this code belongs to Student.
we always want our array to be as size of its elements, when it is empty so it could point to NULL, when we wanted to add value, we extend it.
when we have counter and increment, it is better to use for for better code readability, and more importantly, our condition should be !feof(f) because we want to read until the file ends.
probably the most important problem is this: you allocate memory less than we need, the i variable always contains last size, if we want to expand array, we should allocate memory with a size of i+1.
as you can see it is much simpler to just use . operator instead of getting address and use ->.
we allocate memory but never used it and freed it, I assumed that we want to use it and free it later after ending of function so I returned the pointer, otherwise, we have memory leak.

last note: allocating 20 bytes for a char string isn't always safe and getting input with sscanf could cause memory problems, you should consider using fgets or getline, because they are safe.

good luck.

`"%s"` in fscanf is not a safe specifier, it's prone to buffer overflow, use `"%19s"`. — anastaciu, Jul 01 '20 at 20:36
you are right, i mentioned that it is not safe, thank you for completion. — roozbeh sharifnasab, Jul 01 '20 at 20:39
You did, I missed that, still, as I mentioned, using `%19s` instead of `%s` solves the issue. — anastaciu, Jul 01 '20 at 20:59

guyr79 · Answer 2 · 2020-07-01T20:40:57.677

in the first reallocation allocated size is calculated by multiplying with i = 0, thus allocating size of 0. Also, studenName is already an address as it is an array, so the '&' before this variable is not needed.

It can be better written, but sticking with the original code a better version is:

i = 0;
while (!feof(f)) {
    fscanf(f, "%d, %s\n", &(ptr+i)->code, (ptr+i)->studentName);
    ++i;   
    ptr = realloc(ptr, sizeof(struct Students) * (i+1));
}

Final point which is worth to note: performance-wise, realloc is a costly action. Consider allocating more than 1 allocation unit per loop cycle. Memory here is not nearly as expensive

See my comment in the other answer. – anastaciu Jul 01 '20 at 20:50 — anastaciu, Jul 01 '20 at 20:50

Copy the file content to a struct

2 Answers2