1

I am trying to parse the contents of a file into an array of a stucture. I only include the parts of the code that are of interest. The issue in the code lies in my attempting to access (*data[i]).title. My code gets stuck on this line but I dont have any idea why, I have tried messing around with syntax but haven't gotten anywhere.

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

const int amountOfLines = 100, maxLineLength = 150;

//A structure to store different elements of data for each game review
typedef struct Element Element;
struct Element{
    char title[60];
};

void parsingFile(FILE * file, Element * data[amountOfLines]);

int main(){
    FILE * ignData = fopen("t4_ign.csv", "r");    
    Element * data[amountOfLines];
    parsingFile(ignData, data);
}

void parsingFile(FILE * file, Element * data[amountOfLines]){
    for(int i = 0; i < 1; i++){
        char buffer[maxLineLength];
        fgets(buffer, maxLineLength, file);
        char * token = strtok(buffer, ",");
        strcpy((*data[i]).title, token);
        printf("%s is the title\n", (*data[i]).title);
    }
}

1 Answers1

0

There's a number of issues.

    Element * data[amountOfLines];
    parsingFile(ignData, data);

data is allocated but never initialized. It will contain garbage. So *data[i] is dereferencing garbage. You either need to pre-initialize data with Elements, or make parsingFile responsible for allocating Elements and placing them on data.

As the caller does not know how many lines will be in parsingFile, it would also make sense to let parsingFile allocate and return the Elements array and their elements.


        fgets(buffer, maxLineLength, file);
        char * token = strtok(buffer, ",");
        strcpy((*data[i]).title, token);

The maxLineLength is 150. buffer and thus token could be as large as 150 bytes, but title can only hold 60.

Instead of hard coding the size of the title, make the title a character pointer and strdup the token.

typedef struct {
    char *title;
} Element;

...

   // Use the size of the buffer so the buffer size and read size are
   // never out of sync.
   fgets(buffer, sizeof(buffer), file);
   char * token = strtok(buffer, ",");
   (*data[i]).title = strdup(token);

Here's one way to rewrite this code. Rather than using fixed size constants, or awkwardly passing around arrays and their size, it uses a struct to collect them together. All operations on that struct are in functions to make it easy to work with and debug.

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

typedef struct {
    // Store a pointer rather than a fixed size string.
    char *title;
} Element;

// A single struct to encapsulate the array and it size.
typedef struct {
    // Store a list of pointers, not copies of the elements.
    //
    // This could be made generic by storing a `void **`, but that's advanced.
    Element **data;
    // Also store the size of the data.
    size_t size;
} Elements;

// Have a function for creating structs, even if it's simple now.
// It might get complicated later. This is separate from initializing.
// For example, we don't allocate memory for title. That would be a
// waste.
Element *Element_new() {
    return malloc(sizeof(Element));
}

Elements *Elements_new() {
    return malloc(sizeof(Elements));
}

void Elements_init(Elements *elements) {
    elements->data = NULL;
    elements->size = 0;
}

Element *Elements_get(Elements *elements, size_t i) {
    assert( i < elements->size);
    return elements->data[i];
}

// Isolate the error-prone process of growing the array.
// This simplifies using Elements and offers an opportunity to
// optimize later.
void Elements_grow(Elements *elements, size_t by) {
    size_t new_size = elements->size + by;
    elements->data = realloc(elements->data, sizeof(Element*) * new_size);
    elements->size += by;
}

// Same here, isolate a discrete process.
void Elements_append(Elements *elements, Element *element) {
    Elements_grow(elements, 1);
    elements->data[elements->size-1] = element;
}

Elements *parsingFile(FILE * file) {
    // Use descriptive names.
    Elements *elements = Elements_new();
    Elements_init(elements);

    // Allocate the buffer outside the loop to be reused.
    // An optimizer will probably do this anyway, but it's
    // easier to understand.
    //
    // Use BUFSIZ. I/O happens in blocks and BUFSIZ is often
    // close to the block size.
    char buffer[BUFSIZ];

    // Read until you're done, not a fixed number of times.
    //
    // Use the size of the buffer so the buffer and read sizes will
    // never get out of sync.
    while( fgets(buffer, sizeof(buffer), file) != NULL ) {
        char *token = strtok(buffer, ",");

        // We don't need to initialize this Element because we're
        // going to replace the title with our own pointer anyway.
        Element *elem = Element_new();

        // Copy the token from the buffer to the struct.
        // This uses exact enough memory.
        elem->title = strdup(token);

        Elements_append(elements, elem);
    }

    return elements;
}

int main() {
    FILE *ignData = fopen("t4_ign.csv", "r");
    Elements *elements = parsingFile(ignData);

    for( size_t i = 0; i < elements->size; i++ ) {
        printf("%zu: %s\n", i, Elements_get(elements, i)->title);
    }
}
Schwern
  • 153,029
  • 25
  • 195
  • 336