Trying to store values from file into array in C

Question

I'm learning C and trying to write a program to hold music information. I'm reading from a file with the following contents:

Hello,Adele,2015
Sorry,Justin Bieber,2015
X Gon Give It To Ya,DMX,2002

And trying to do a fairly basic operation - look at each line until EOF, grab the content, tokenize using ',' as delimiter, store each part of the string into 3 variables, store those 3 variables into the global array using the global index, which is defined in main like this

Song SongList[1024];
int globalCounter;
globalCounter = 0;

The function I wrote looks like this, where fp is a successfully opened file, SongList is an array of Song structures and globalCounter is the global index of current position in array.

int getFileData(FILE *fp, Song *SongList, int globalCounter) {
    int newCount = globalCounter;
    char fileOut[1024];
    int lineno = 0;

    while (!feof(fp)) {
        if (fgets(fileOut, 1024, fp) != NULL) {
            newCount++;
            char *tokenizer;
            char *fileTitle;
            char *fileArtist;
            char *fileYear;

            tokenizer = strtok(fileOut, ",");
            int counter = 0;
            fileTitle = tokenizer;
            counter++;
            while (tokenizer != NULL) {
                tokenizer = strtok(NULL, ",");
                if (counter == 1)
                    fileArtist = tokenizer;
                if (counter == 2)
                    fileYear = tokenizer;
                counter++; 
            }

            SongList[newCount].title = fileTitle;
            SongList[newCount].artist = fileArtist;
            SongList[newCount].year = fileYear;
            // prints the right values
            printf("%i\n", newCount);
            printf("TITLE: %s\n", SongList[newCount].title); 
            printf("ARTIST: %s\n", SongList[newCount].artist);
            printf("YEAR: %s\n", SongList[newCount].year);
        }
    }
    return newCount;
}

It looks like it works fine, however, when I try to do a simple print of contents before the return statement, I get garbage return data.

int counter = 0;
while (counter < newCount) {
    printf("%s, %s, %s", SongList[newCount].title, SongList[newCount].artist, SongList[newCount].year);
    counter++;
}

yields:

X Gon Give It To Ya, DMX, 2002X Gon Give It To Ya, DMX, 2002X Gon Give It To Ya, DMX, 2002

When I try to use pretty much the exact same while loop in another function, I get even more garbage data output.

(null), (null), (null), , ╨╦a, , DMXm

The song structure looks like this

typedef struct Song {
    char *title;
    char *artist;
    char *year; 
} Song;

I suspect the problem has a simple solution: something to do with type definitions of variables, missing */&, or not ending with '\0' - I'm not sure what is causing this though.

Answer 1

Instead of while (!feof(fp)) { if (fgets(fileOut, 1024, fp) != NULL) { just use:

   while (fgets(fileOut, 1024, fp) != NULL) {

Note that your parsing with strtok is sloppy: the artist will start with a space, the year will start with a space and include the final line feed. Using a pointer and parsing by hand would be more precise.

You must store copies of the strings:

       SongList[newCount].title = strdup(fileTitle);
       SongList[newCount].artist = strdup(fileArtist);
       SongList[newCount].year = strdup(fileYear);

You may want to free these strings when you are done processing the data, unless you exit the program, in which case free is not necessary, but still recommended to help track potential memory leaks.

Also increment newCount++; after successfully storing the entry.

According to your description, you should also store newCount to the global variable globalCounter.

Your printing loop should use counter instead of newCount:

    int counter = 0;
    while (counter < newCount) {
        printf("%s, %s, %s\n",
               SongList[counter].title, 
               SongList[counter].artist, 
               SongList[counter].year);
        counter++;
    }

Here is a corrected version:

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

typedef struct Song {
    char *title;
    char *artist;
    char *year; 
} Song;

Song SongList[1024];
int globalCounter = 0;

int getFileData(FILE *fp, Song *SongList, int *globalCounter) {
    int newCount = *globalCounter;
    char fileOut[1024];

    while (newCount < 1024 && fgets(fileOut, 1024, fp) != NULL) {
        char *p = fileOut;
        char *fileTitle;
        char *fileArtist;
        char *fileYear;

        fileTitle = p;
        if ((p = strchr(p, ',')) == NULL)
            continue;
        *p++ = '\0';
        p += strspn(p, " \t");  /* skip blank characters */
        fileArtist = p;
        if ((p = strchr(p, ',')) == NULL)
            continue;
        *p++ = '\0';
        p += strspn(p, " \t");  /* skip blank characters */
        fileYear = p;
        p += strcspn(p, ",\n");  /* skip to ',' or '\n' or end of string */
        *p = '\0';

        SongList[newCount].title = strdup(fileTitle);
        SongList[newCount].artist = strdup(fileArtist);
        SongList[newCount].year = strdup(fileYear);

        // prints the right values
        printf("%i\n", newCount);
        printf("TITLE: %s\n", SongList[newCount].title); 
        printf("ARTIST: %s\n", SongList[newCount].artist);
        printf("YEAR: %s\n\n", SongList[newCount].year);
        newCount++;
    }
    return *globalCounter = newCount;
}

int main() {
    FILE *fin;

    if ((fin = fopen("test.txt", "r")) != NULL) {
        getFileData(fin, SongList, &globalCounter);
        fclose(fin);
    }
    for (int counter = 0; counter < globalCounter; counter++) {
        printf("%s, %s, %s\n",
            SongList[counter].title,
            SongList[counter].artist,
            SongList[counter].year);
        counter++;
    }
    return 0;     
}

Answer 2

You need to duplicate the string. You do not just keep a pointer to the strtok return. (And you should check the return value of strtok and strdup for errors and deallocate the memory when done with a Song record). You are iterating the index too soon (skipping index 0).
And your testing of the return value is indexing the same last position in the for loop.

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

typedef struct Song 
{
    char* title;
    char* artist;
    char* year; 
} Song;

Song SongList[1024];
int globalCounter = 0;


int getFileData(FILE* fp, Song* SongList, int globalCounter)
{
    int newCount = globalCounter;
        char fileOut[1024];
        int lineno = 0;
        while (!feof(fp))
        {
            if (fgets(fileOut,1024,fp) != NULL)
            {
                char *tokenizer;
                //char* fileTitle;
                //char* fileArtist;
                //char* fileYear;

                tokenizer = strtok(fileOut, ",");
                int counter = 0;
                SongList[newCount].title = strdup( tokenizer );
                //fileTitle = tokenizer;
                counter++;
                while(tokenizer != NULL)
                {
                    tokenizer = strtok(NULL, ",");
                    if(counter == 1)
                        SongList[newCount].artist = strdup( tokenizer );
                    //    fileArtist = tokenizer;
                    if(counter == 2)
                        SongList[newCount].year = strdup( tokenizer );
                    //    fileYear = tokenizer;
                    counter++; 
                }

                //SongList[newCount].title = fileTitle;
                //SongList[newCount].artist = fileArtist;
                //SongList[newCount].year = fileYear;
                // prints the right values
                printf("%i\n",newCount);
                printf("TITLE: %s\n", SongList[newCount].title); 
                printf("ARTIST: %s\n", SongList[newCount].artist);
                printf("YEAR: %s\n", SongList[newCount].year);
                newCount++;
            }
        }
    return newCount;
}

int main()
{
    FILE * fin = fopen( "test.txt", "rt" );
    int newCount = getFileData( fin, SongList, globalCounter );
    int counter = 0;
    while(counter < newCount){
        printf("%s, %s, %s",SongList[counter].title,SongList[counter].artist,SongList[counter].year);
        counter++;
    }

}

Test:

1212:/tmp$ g++ test.cpp  && ./a.out 
0
TITLE: Hello
ARTIST: Adele
YEAR: 2015

1
TITLE: Sorry
ARTIST: Justin Bieber
YEAR: 2015

2
TITLE: X Gon Give It To Ya
ARTIST: DMX
YEAR: 2002

Hello, Adele, 2015
Sorry, Justin Bieber, 2015
X Gon Give It To Ya, DMX, 2002