CODEWITHSUNDEEP
cfilestruct
tibi
tibi

Reputation: 186

Copy the file content to a struct

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

Upvotes: 1

Views: 87

Answers (2)

roozbeh sharifnasab
roozbeh sharifnasab

Reputation: 303

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. as you can see it is much simpler to just use . operator instead of getting address and use ->.
  6. 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.

Upvotes: 2

guyr79
guyr79

Reputation: 167

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

Upvotes: 1

Related Questions