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.

Every second question is about it... allocate your strings! — Eugene Sh.
– Eugene Sh., Commented Jan 14, 2016 at 22:09
@EugeneSh. I originally allocated like this: char tokenPtr[40]; char fileTitle[40]; char fileArtist[40]; char fileYear[40]; but end up getting lots of errors like this error: incompatible types when assigning to type 'char[40]' from type 'char [star]' tokenPtr = strtok(buffer, ","); which means strtok output is incompatible with char [40] - leaving it as char[star] x worked but is causing what I talked about in the question — unicornication
– unicornication, Commented Jan 14, 2016 at 22:17
Each of your Song items has to have it's fields allocated. And to reduce the chance of being downvoted - remove justin Bieber from your list :) — Eugene Sh.
– Eugene Sh., Commented Jan 14, 2016 at 22:18
In that printf statement, you're using newcount where I believe you should be using counter (otherwise you're just printing the same song several times). — maxton
– maxton, Commented Jan 14, 2016 at 22:25

chqrlie · Accepted Answer · 2016-01-14 23:19:32Z

3

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;     
}

edited Jan 14, 2016 at 23:19

answered Jan 14, 2016 at 22:19

chqrlie

152k12 gold badges145 silver badges231 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

unicornication Over a year ago

Amazing answer!! Thank you :-) In using your code though, how do you get around this error? music.c:108:16: warning: assignment makes pointer from integer without a cast [enabled by default] if ((p = strchr(p, ',') == NULL)) ^ music.c:113:16: warning: assignment makes pointer from integer without a cast [enabled by default] if ((p = strchr(p, ',') == NULL))

chqrlie Over a year ago

@unicornication: oops! my bad, there were 2 typos, I fixed the answer. btw, I did not compile the code, sorry for any other errors.

unicornication Over a year ago

It runs fine! The only issue is two missing parenthesis after the NULL) 's, I made an attempt to edit your answer, but 6 edit character limit. Thank you very much!

chqrlie Over a year ago

@unicornication: I edited the answer already, if ((p = strchr(p, ',')) == NULL) should be fine, no missing parentheses anymore.

George Houpis · Accepted Answer · 2016-01-14 22:27:48Z

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

You forgot the ugly redundant end of file test: while (!feof(fp))

Collectives™ on Stack Overflow

Trying to store values from file into array in C

2 Answers 2

4 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related