Reading lines from file

Question

I am trying to read strings and integers from a simple text file to my array. But the problem is that I get some random characters in a line in the middle of my list. It probably has to do with a newline problem, but I am not sure. The text file looks like this:

4
Mr Tambourine Man
Bob Dylan
1965
Dead Ringer for Love
Meat Loaf
1981
Euphoria
Loreen
2012
Love Me Now
John Legend
2016

The first number (4), indicates how many songs there are in the list. I have made a struct which will be able to hold the songs and dynamically allocate memory for each pointer. Struct:

typedef struct Song {
    char *song;
    char *artist;
    int *year;
} Song;

Allocated:

Song *arr;
arr = (Song*)malloc(sizeof(Song));

Function:

int loadFile(char fileName[], Song *arr, int nrOf) {

    FILE *input = fopen(fileName, "r");

    if (input == NULL) {
        printf("Error, the file could not load!\n");
    } else {
        int i = 0;
        fscanf(input, "%d\n", &nrOf);

        for (int i = 0; i < nrOf; i++) {
            arr[i].song = (char*)malloc(sizeof(char));
            arr[i].artist = (char*)malloc(sizeof(char));
            arr[i].year = (int*)malloc(sizeof(int));
            fgets(arr[i].song, 100, input);
            fgets(arr[i].artist, 100, input);
            fscanf(input, "%d\n", arr[i].year);
        }
        printf("The file is now ready.\n");
        fclose(input);
    }

    return nrOf;
}

Are you able to find the problem? Or do you have a better solution?

Answer 1

Your memory allocation is incorrect. The structure should have char arrays for the song and artist names and an int for the year, and you should modify your API to return the array and its size to the caller:

int loadFile(const char *fileName, Song **arr, int *numberp);

Here is a corrected and simplified of your program:

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

typedef struct Song {
    char song[100];
    char artist[100];
    int year;
} Song;

/* call as
   if (loadFile(fileName, &songs, &songs_size) < 0) {
       // deal with error...
   }
*/

int loadFile(const char *fileName, Song **arrp, int *numberp) {
    FILE *input;
    Song *arr;
    int i, nrOf;

    input = fopen(fileName, "r");
    if (input == NULL) {
        fprintf(stderr, "Cannot open file %s\n", filename);
        return -1;
    } else {
        if (fscanf(input, "%d\n", &nrOf) != 1) {
            fprintf(stderr, "%s: missing number of items\n", filename);
            fclose(intput);
            return -1;
        }
        arr = calloc(sizeof(*arr), nrOf);
        if (arr == NULL) {
            fprintf(stderr, "cannot allocate memory for %d items\n", nrOf);
            fclose(intput);
            return -1;
        }    
        for (int i = 0; i < nrOf; i++) {
            char cc;
            if (fscanf(input, "%99[^\n]%*c%99[^\n]%*c%d%c",
                       sarr[i].song, arr[i].artist,
                       &arr[i].year, &cc) != 4 || cc != '\n') {
                fprintf(stderr, "%s: invalid format for item %d\n",
                        filename, i);
                break;
            }
        }
        printf("The file is now ready.\n");
        fclose(input);
        *arrp = arr;
        *numberp = i;
        return i;
    }
}

Answer 2

• First Issue:

  arr[i].song = (char*)malloc(sizeof(char));
  arr[i].artist = (char*)malloc(sizeof(char));
  

  Are only allocating 1 byte for your char* pointers, song and artist. You can allocate a size for this:

  arr[i].song = (char*)malloc(100 * sizeof(char)); /* or malloc(100) */
  arr[i].artist = (char*)malloc(100 * sizeof(char));
  

  Or you can simply malloc() enough space from you buffer:

  char buffer[100];
  fgets(buffer, 100, input);
  /* check for failure, remove newline */
  arr[i].song = malloc(strlen(buffer)+1);
  /* check error from malloc */
  strcpy(arr[i].song, buffer);
  

  Or even use strdup():

  arr[i].song = strdup(buffer);
  

  Which is a substitute for malloc()/strcpy().

  Note: You can also read Do I cast the result of malloc?.

• Second Issue:

  Your current struct:

  typedef struct Song {
      char *song;
      char *artist;
      int *year;
  } Song;
  

  Can be simplified to:

  typedef struct {
      char *song;
      char *artist;
      int year;
  } Song;
  

  Because year does not need to be a pointer. Easier to manage if its just an int. This avoids having to do allocations like:

  arr[i].year = (int*)malloc(sizeof(int));
  

• Other Recommendations: You should check the return of fscanf() and fgets() as its safe to do this. It helps just incase your file will have incorrect data. This goes the same for malloc(), which can return NULL is unsuccessfully allocated on the heap.

Here is some code with the above considerations in mind:

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

#define SIZE 100

typedef struct {
    char *song;
    char *artist;
    int year;
} Song;

Song *create_array(FILE *input, int *nrof);
void load_data(Song *arr, FILE *input, int nrof);
void print_free_data(Song *arr, int nrof);
void get_buffer(char buffer[], FILE *input);

int main(void) {
    FILE *input;
    Song *arr;
    int nrof;

    input = fopen("artist.txt", "r");
    if (input == NULL) {
        fprintf(stderr, "Error reading file\n");
        exit(EXIT_FAILURE);
    }

    arr = create_array(input, &nrof);

    load_data(arr, input, nrof);

    print_free_data(arr, nrof);

    fclose(input);

    return 0;
}

Song *create_array(FILE *input, int *nrof) {
    Song *arr;

    if (fscanf(input, "%d ", nrof) != 1) {
        fprintf(stderr, "Cannot find number of songs\n");
        exit(EXIT_FAILURE);
    }

    arr = malloc(*nrof * sizeof(*arr));
    if (arr == NULL) {
        fprintf(stderr, "Cannot allocate %d spaces for array\n", *nrof);
        exit(EXIT_FAILURE);
    }

    return arr;
}

void load_data(Song *arr, FILE *input, int nrof) {
    char buffer[SIZE];

    for (int i = 0; i < nrof; i++) {
        get_buffer(buffer, input);
        arr[i].song = malloc(strlen(buffer)+1);
        if (arr[i].song == NULL) {
            fprintf(stderr, "Cannot allocate song\n");
            exit(EXIT_FAILURE);
        }
        strcpy(arr[i].song, buffer);

        get_buffer(buffer, input);
        arr[i].artist = malloc(strlen(buffer)+1);
        if (arr[i].artist == NULL) {
            fprintf(stderr, "Cannot allocate artist\n");
            exit(EXIT_FAILURE);
        }
        strcpy(arr[i].artist, buffer);

        if (fscanf(input, "%d ", &arr[i].year) != 1) {
            fprintf(stderr, "Cannot find year for Song: %s Album: %s\n", 
                             arr[i].song, arr[i].artist);
            exit(EXIT_FAILURE);
        }
    }
}

void get_buffer(char buffer[], FILE *input) {
    size_t slen;

    if (fgets(buffer, SIZE, input) == NULL) {
        fprintf(stderr, "Error from fgets(), line not read\n");
        exit(EXIT_FAILURE);
    }

    slen = strlen(buffer);
    if (slen > 0 && buffer[slen-1] == '\n') {
        buffer[slen-1] = '\0';
    } else {
        fprintf(stderr, "Too many characters entered\n");
        exit(EXIT_FAILURE);
    }
}

void print_free_data(Song *arr, int nrof) {
    for (int i = 0; i < nrof; i++) {
        printf("%s\n%s\n%d\n\n", arr[i].song, arr[i].artist, arr[i].year);

        free(arr[i].song);
        arr[i].song = NULL;

        free(arr[i].artist);
        arr[i].artist = NULL;
    }

    free(arr);
    arr = NULL;
}

Which Outputs correct data:

Mr Tambourine Man
Bob Dylan
1965

Dead Ringer for Love
Meat Loaf
1981

Euphoria
Loreen
2012

Love Me Now
John Legend
2016

Answer 3

This is wrong:

arr[i].song = (char*)malloc(sizeof(char));
arr[i].artist = (char*)malloc(sizeof(char));

You are only allocating buffers of size 1, there's no scaling. This gives you undefined behavior when you overrun the buffers by loading more data into them than they can hold.

I would expect those to read:

arr[i].song = malloc(100);

and so on. Note that no cast is necessary, and sizeof (char) is always 1.

Also, this:

arr[i].year = (int*)malloc(sizeof(int));

is super-strange. There's absolutely no reason to dynamically allocate a single integer, just make the field an int and store the value there directly.