2

I am reading in from file into structure and came across problem. I have test file where first letter defines name of structure second number tells me how many nodes does it have and rest numbers are nodes. File example:

A 4 1 2 3 4
B 5 1 2 3 9 8 
C 3 1 2 3 

So for example structure should be this: name->A; numberOfNodes->4; nodes->{1,2,3,4}. My structure where I save each row is this:

struct mystruct{
char name[1];
int numberOfNodes;
int nodes[];
};

my function so far:

lines = lineCount(courses); //calculates how many rows file has
struct courses course[lines];
co = fopen(courses, mode);
if(co == NULL){
    printf("Can't find the files.");
    exit(1);
}else{
    for(i = 0; i < lines; i++){
        fscanf(co, "%1s %d \n", &current, &id1); //Doesnt have any problems reading these two parameters;
        for(j = 0 ; j < id1; j++){ 
            fscanf(co, "%d", &course[i].nodes[j]); //Have no idea how to store array =/
        }
        strcpy(course[i].courseName, current);
        course[i].numberOfNodes = id1;
    }
}

EDIT: I am sorry for confusing you guys, it allocates integers just fine but instead of outputting same thing it outputs something like this:

A 4 69 72 1 2
B 5 20 45 7 3 1 
C 3 2 45 1 

I think that this bit of code doesnt do what I want it to do:

        for(j = 0 ; j < id1; j++){ 
            fscanf(co, "%d", &course[i].nodes[j]); //Have no idea how to store array =/
        }

Would appreciate any help!

Shepard
  • 1,111
  • 5
  • 16
  • 32
  • use a temporary variable and read in it, afterwards make it course[i].nodes[j] = tempVal, as per my answer, that got downvoted. Or directly try &(course[i].nodes[j]) to force it to get the correct pointer location. But allocating the array as dynamic is the way to go – LonWolf Dec 12 '12 at 09:08
  • 1
    You should use `char name` instead of `char name[1]`. – kmkaplan Dec 12 '12 at 09:11

4 Answers4

2

Your code as it stands doesn't allocate any memory for the integer array, the int nodes[] is called a flexible array member it has its uses and it doesn't reserve any memory on its own, you will need to allocate dynamic memory for the nodes array:

struct mystruct {
    char name[1];
    int numberOfNodes;
    int *nodes;
};
...
fscanf(co, "%c %d \n", &current, &id1);   
course[i].nodes = malloc(sizeof(int)*id1);

Note that the %1s format specifier scans a 1 character string, it will add a null-terminating byte afterwards, so you should use %c instead to read just one character.

Note1: don't forget to free() all the memory you have allocated when you're done, example

free(course[i].nodes);

Note2: The idiomatic way to allocate memory in C is:

malloc(num_of_elements * sizeof *ptr_to_type); 

I didn't introduce that here to avoid confusion, also note that I personally don't prefer to cast the result of malloc(), there are some good reasons for that: Do I cast the result of malloc?

Community
  • 1
  • 1
iabdalkader
  • 17,009
  • 4
  • 47
  • 74
  • If you reall want to provide a correct answer by the book then complete your response: use `free(nodes)` where nodes is the pointer to int. Also, malloc function returns a `void*` pointer that can be treated by any kind of pointer variable as valid. Adding an explicit cast beforehand - `nodes = (int*) malloc(sizeof(int)*numberOfNodes);` - instead or relying on default casts not only makes code more readable but ensures you get what you asked for. – LonWolf Dec 12 '12 at 09:20
  • @LonWolf first there's a note to use `free()` second, in C [you should *not* cast the result of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Again this is a **C** answer not C++. – iabdalkader Dec 12 '12 at 09:22
1
#include <stdio.h>
#include <stdlib.h>

struct mystruct {
    struct mystruct *next;
    char name; /* No need to dim array as name[1] */
    int numberOfNodes;
    int nodes[];
};

int main(void)
{
    /* Don't like VLA's, I used a list */
    struct mystruct *curr, *first = NULL, *prior = NULL;
    char s[256], *p;
    FILE *f;
    int i;

    /* Open (No need to count lines before) */
    f = fopen("data", "r");
    if (f == NULL) {
        perror("fopen");
        exit(EXIT_FAILURE);
    }
    /* Fill */
    while (fgets(s, sizeof(s), f) != NULL) {
        i = (int)strtol(&s[1], &p, 10);
        if (i == 0) continue; /* Skip blank or 0 node */
        /* Flexible array must be alloced with parent */
        curr = malloc((sizeof *curr) + (sizeof(int) * (size_t)i));
        curr->next = NULL;
        curr->name = s[0];
        curr->numberOfNodes = i;
        for (i = 0; i < curr->numberOfNodes; i++) {
            curr->nodes[i] = (int)strtol(p, &p, 10);
        }
        if (prior) {
            prior->next = curr;
        } else {
            first = curr;
        }
        prior = curr;
    }
    fclose(f);
    /* Print */
    curr = first;
    while (curr) {
        printf("%c %d", curr->name, curr->numberOfNodes);
        for (i = 0; i < curr->numberOfNodes; i++) {
            printf(" %d", curr->nodes[i]);
        }
        printf("\n");
        curr = curr->next;
    }
    /* Free */
    while (first) {
        curr = first->next;
        free(first);
        first = curr;
    }
    return 0;
}
David Ranieri
  • 39,972
  • 7
  • 52
  • 94
0

Your problem is that you do not know before how many numbers follow the letter. So you either can say not more than 1000 or whatever you find "sufficient". Or you store this number in a kind of linked structure one after the other. That later is much easier done if you use e.g libraries which provide linked lists. One example of it would be glib.

If you want to store in arrays you have to use stuff for dynamically allocating space for the arrays. You will need some combination of malloc/realloc/free.

Storing the iteme of the array can be done as you printed &course[i].nodes[j] will do, but of course you need enough space for really accessing nodes[j]

Regards

Friedrich
  • 5,916
  • 25
  • 45
-1

If you are using standard C/C++ you need to initialize the array prior to assigning values, and make sure you get the right pointer when you fscanf()!

struct mystruct{
char name[1];
int numberOfNodes;
int* nodes;
};

then in the else part of your if:

else{
    for(i = 0; i < lines; i++){
        fscanf(co, "%1s %d \n", &current, &id1); //Doesnt have any problems reading these two parameters;
        course[i].nodes = new int[id1]; //init array length of id1 value
        for(j = 0 ; j < id1; j++){ 
            int tempNode;
            fscanf(co, "%d", &tempNode); 
            course[i].nodes[j] = tempNode; //allocate the node value to the correct location. 
        }
        strcpy(course[i].courseName, current);
        course[i].numberOfNodes = id1;
    }

Alternatively you should be able to bypass a temporary value by using:

fscanf(co, "%d", &(course[i].nodes[j]));

But i think some compilers may fail to see this as correct. Using a temp value is better - my opinion - because of the structure of your data.

At the end don't forget to loop through the course[] vector and delete (course[i].nodes)

If you want pure C, you will need to use malloc() which can be a pain to handle - well not as pretty as new, delete.

Anyway, i suggest using STL vector for a much more easier memory management, relief the stress of pointer allocation and use an elegant data management (iteration, addition, deletion) by using the predefined operator functions - push_back, pop_back, insert, erase.

http://www.cplusplus.com/reference/vector/vector/


I added '()' to some bits of code to underline the operator priority.

LonWolf
  • 132
  • 4
  • that's not C, that's mixed C/C++ C doesn't have `new`, `delete` and definitely no `vector` – iabdalkader Dec 12 '12 at 09:03
  • I wasn't ready with the answer. I want to give a C++ compliant solution because it's more ellegant. Thanks for the downvote tho.. – LonWolf Dec 12 '12 at 09:05
  • The question is tagged `C` not `C++` a C++ solution is not required, the down vote means this answer is not useful and is not meant to offend you. – iabdalkader Dec 12 '12 at 09:06
  • @LonWolf: more elegant? it's your point of view, malloc can be a pain to handle is also your point of view, question is tagged C and your suggestions can confuse op – David Ranieri Dec 12 '12 at 12:08