array of struct read from file in C

Question

I want to read csv file in to array of structs with NULL ending.

test.txt:

11,345,1,k
12,34.23,3,v
13,13.4,5,x
14,15.7,6,c

My code:

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

struct csv_line {
  int value1;
  float value3;
  int value2;
  char char1;
};

struct csv_line *line_pointer = NULL;

int main(void) {
    FILE *csvFile = NULL;
    csvFile = fopen("test.txt", "r");
    if (csvFile == NULL)
      exit(1);

    size_t i = 0;
    while (1) {
    line_pointer = (struct csv_line *)realloc(line_pointer, sizeof(struct csv_line) * (i + 1));
    if (fscanf(csvFile, "%d,%f,%d,%c",
            &(line_pointer + i)->value1,
            &(line_pointer + i)->value3,
            &(line_pointer + i)->value2,
            &(line_pointer + i)->char1) != EOF) {
      i++;
    } else{
        (line_pointer + i)->value1 = 0;
        (line_pointer + i)->value3 = 0.0;
        (line_pointer + i)->value2 = 0;
        (line_pointer + i)->char1 = '\0';
        break;
     }
    }
    fclose(csvFile);
    int k = 0;
    while('\0' != (line_pointer + k)->char1) {
        printf("%d, %.2f, %d, %c",
                (line_pointer + k)->value1,
                (line_pointer + k)->value3,
                (line_pointer + k)->value2,
                (line_pointer + k)->char1);
        printf("\n");
        k++;
    }

    free(line_pointer);

    return EXIT_SUCCESS;
}

Questons:

1. Is it possible to move fscanf condition inside while-loop condition instead of endless (1) ?
2. How to set "end" of array with single NULL like "line_pointer[i] = NULL" in else statement?

Answer 1

user3629249's answer is good. The only thing I have to add:

struct csv_line *line_pointer = NULL;

// ...

&(line_pointer + i)->value1

is awkward. Why go through the pointer addition acrobatics? Why not just

&line_pointer[i].value1

Answer 2

The following proposed code:

1. cleanly compiles
2. performs the desired functionality
3. takes into account the comments to the OPs question
4. properly checks for errors when calling malloc() and realloc() and fopen() and fscanf()
5. the while() loop uses the function: fscanf() rather than a '1' as the loop condition
6. uses the value in i to determine when the end of the data is reached
7. sets the last instance of the struct to NULL per the OPs request
8. per Reinderien's answer and for ease of readability, statements like &(line_pointer + i)->value1 (which uses pointer addition) could be replaced with: &(line_pointer[i].value1 (which uses indexing)

and now, the proposed code:

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

struct csv_line {
  int value1;
  float value3;
  int value2;
  char char1;
};

struct csv_line *line_pointer = NULL;

int main(void) 
{
    FILE *csvFile = NULL;
    csvFile = fopen("test.txt", "r");
    if (csvFile == NULL)
    {
        perror( "fopen failed" );
        exit(1);
    }

    size_t i = 1;
    line_pointer = malloc( sizeof(struct csv_line) * i );
    if( ! line_pointer )
    {
        perror( "malloc failed" );
        fclose( csvFile );
        exit( EXIT_FAILURE );
    }

    while ( (fscanf(csvFile, "%d,%f,%d,%c",
                &(line_pointer + i)->value1,
                &(line_pointer + i)->value3,
                &(line_pointer + i)->value2,
                &(line_pointer + i)->char1) ) == 4 )
    {
        i++;
        struct csv_line *temp = realloc(line_pointer, sizeof(struct csv_line) * i );
        if( ! temp )
        {
            perror( "realloc failed" );
            free( line_pointer );
            fclose( csvFile );
            exit( EXIT_FAILURE );
        }
    }

    fclose(csvFile);

    // per the request that the last entry contains NULL
    memset( &line_pointer[i], 0, sizeof( struct csv_line ) );

    for( size_t k=0; k < (i-1); k++ ) 
    {
        printf("%d, %.2f, %d, %c",
                (line_pointer + k)->value1,
                (line_pointer + k)->value3,
                (line_pointer + k)->value2,
                (line_pointer + k)->char1);
        printf("\n");
    }

    free(line_pointer);

    return EXIT_SUCCESS;
}

you could try this loop instead:

size_t i = 0;

do
{
    i++;
    struct csv_line *temp = realloc(line_pointer, sizeof(struct csv_line) * i );
    if( ! temp )
    {
        perror( "realloc failed" );
        free( line_pointer );
        fclose( csvFile );
        exit( EXIT_FAILURE );
    }
} while ( (fscanf(csvFile, "%d,%f,%d,%c",
            &(line_pointer + i)->value1,
            &(line_pointer + i)->value3,
            &(line_pointer + i)->value2,
            &(line_pointer + i)->char1) ) == 4 );

so there is only one place allocating memory

Answer 3

I'll answer your questions in reverse order:

How to set "end" of array with single NULL like line_pointer[i] = NULL in else statement?

You can (with memset, as shown in other answers), but it's not guaranteed by the C standard that the float value will be set to zero. I would instead suggest to set every value in the structure to zero as you are already doing.

Is it possible to move fscanf condition inside while-loop condition instead of endless (1)?

You may be able to do this, but I don't recommend it. This would make your code harder to read and comprehend.

Some other issues:

Check the return value of the *alloc family of functions

Right here, a crash could happen if you're out of memory:

line_pointer = (struct csv_line *)realloc(line_pointer, sizeof(struct csv_line) * (i + 1));

You should add a check just like you do with csvFile:

if(!line_pointer)
    exit(1);

Of course, a better error message could be formulated with perror:

if(!line_pointer)
{
    perror("realloc(line_pointer)");
    exit(1);
}

perror will work when fopen fails as well.

Check the return value of fscanf correctly

Instead of checking if you've reached the end-of-file, check whether you didn't read as many variables as expected. This provides a minimal aspect of data validation:

    if (fscanf(csvFile, "%d,%f,%d,%c",
            &(line_pointer + i)->value1,
            &(line_pointer + i)->value3,
            &(line_pointer + i)->value2,
            &(line_pointer + i)->char1) != 4)

Use EXIT_FAILURE instead of 1

As you're using EXIT_SUCCESS to show success, you should go for consistency (and portability) by using EXIT_FAILURE in place of 1 in exit.

Use return instead of exit

In addition to the above, in main you can forego using exit and instead use return. There's really no need to use exit in a program under normal circumstances, anyway.

Don't cast the return value of malloc/calloc/realloc

The return value of realloc can be implicitly converted to any object pointer. It's not necessary to cast it, and in my opinion, avoiding unnecessary casts is a good thing.

Use array and structure access operators instead of pointer arithmetic

To me, this looks odd:

(struct_pointer + ctr)->member

Instead, you can use array access and the . operator:

struct_pointer[ctr].member

This applies equivalently to when you're taking the address of member.

Use consistent indentation

In some places, you use two spaces, while in others you use one and four. I suggest sticking to one indentation style, whatever it is. Personally, I use four or a tab, but you can use whatever you want if it's consistent. See the Linux Kernel Coding Style Guidelines for more information.

Yoda conditions

Backwards, your conditions are. This is hard to read and provides little to no benefit, considering that compilers now warn about typos that Yoda conditions originally solved:

while('\0' != (line_pointer + k)->char1)

Instead, just use != the "normal" way:

while((line_pointer + k)->char1 != '\0')