reading the content of a file into an array of struct

i am writing a function that reads the content of the file into a dynamically-allocated array of "Point" struct.

Code:

---

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




typedef struct{
    double x;
    double y;
}Point;

void readPoints(char* fileName, int* numPoints){
    int i;
    FILE *f = fopen(fileName, "r");


    if(f != NULL){
        fscanf(f, "%d", numPoints);   
        printf("the first line =  %d \n", *numPoints);
       
        Point *poin = (Point*)malloc((*numPoints)*sizeof(Point));


        while (!feof(f)){
            for(i=0; i<(*numPoints);i++){
                fscanf(f, "%f,%f", poin->x, poin->y);   
                printf("%f , %f\n", poin->x, poin->y);


            }
        }
        fclose(f);       
        printf("END\n");
       
        free(poin);
    }
    else{
        perror("Could not open..!\n")    ;
    }
   
   
   
}

---

the file will look something like this
4
2.35,6.14
5.5,7.0
0.0,-55.9
14084.1,39864

the function compiles fine,,,, but it doesnt output the correct result.. ur help will be appreciated..

This is my 30 second review because it's late and I'm tired, so I may be missing some things:

1. Don't use feof() to control your loop (read this link). Instead, check the return value of fscanf to make sure it converted the correct number of items (2 in your case).
2. Why do you have a for loop inside your while loop. It's a bit redundant. I recommend a single while loop that terminates when fscanf doesn't read 2 elements, OR when i is greater than or equal to numPoints, to avoid an overflow. That means your loop condition (when it keeps going) is while fscanf returns 2 AND i is less than numPoints.
3. Read the manual for fscanf (link). You need to use %lf for doubles, %f is for single-precision floats.
4. You need to give fscanf the address of the x coordinate. Yes, poin is a pointer, but poin->x and poin->y are still just plain doubles. Use &(poin->x) and &(poin->y).
5. EDIT: Don't cast the return value of malloc (link).

EDIT: Also, there is a better way to malloc arrays:

Code:

---

SomeType *foo = malloc(num_elements * sizeof(*foo));

---

Notice that I use the variable name (with a * in front to dereference -- the thing it points to, not the pointer) inside the sizeof instead of the type name. That way, if the type of foo ever changes, you only have to change the declaration, the malloc call doesn't need to change too. In your case, it would look like:

Code:

---

Point *poin = malloc((*numPoints) * sizeof(*poin));

---

Code:

---

fscanf(f, "%d", numPoints); 
printf("the first line =  %d \n", *numPoints);

---

Your parameter's * is mixed up.

Quote:

---

Originally Posted by WaltP

Code:

---

fscanf(f, "%d", numPoints); 
printf("the first line =  %d \n", *numPoints);

---

Your parameter's * is mixed up.

---

Nah, looks good to me. It's passed in as an int pointer. That means no * or & when used with fscanf (already a pointer), but you need a * to dereference the int if you're going to printf it with %d.

oops....

anduril462, thank u so much,, i looked at the link u gave,,, they helped me alot,, anddd all those tip were really helpful too...

i found all of the bugs and fixed themm all... thanks again,,

Is it working now? Wouldn't you also need to index poin with "i" since it's an array of struct point? Also, in case you don't know, it is ok to use the same name for an instance of the struct as the type name, in this case you use a capital p for the type but you could do, say: struct Point *point;