Reading the file with comma delimiter using fgets() and strtok()

Question

I have a text file with three fields separated by comma. Example of the content of my text file: 12345, true programming newbie, BS ME To load the file into the program, i used the below code.... my problem is that sometimes the code works and sometimes it doesn't (no error message appears the program just closes itself and doesn't continue). i also observed the text file is blank (nothing is written) it automatically closes itself and doesn't continue. Your help would be highly appreciated. Thanks!

int read(){
     FILE *stream = NULL;
     int ctr;
     char linebuffer[45];
     char delims[]=", ";
     char *number[3];
     char *token = NULL;
     stream = fopen("student.txt", "rt");
     if (stream == NULL) stream = fopen("student.txt", "wt");
     else {
          printf("\nReading the student list directory. Wait a moment please...");
          while(!feof(stream)){            
                ctr=0;
                fgets(linebuffer, 46, stream);
                token = strtok(linebuffer, delims);
                while(token != NULL){
                  number[ctr] = linebuffer;
                  token = strtok(NULL, delims); 
                  ctr++;
                }
          recordCtr++;                      
          }                     
     recordCtr--;
     }
     fclose(stream);
}    

Answer 1

You never copy the token once you've found it. You can't copy linebuffer, as the data in there will be overwritten as the next line is loaded.

This line:

number[ctr] = linebuffer;

should reference token to save the most recently found token, but it doesn't. It should probably read something like¹:

strcpy(number[ctr], token);

but then you'd have to change the declaration to make sure there's space:

char number[3][32];

Obviously, this introduces a buffer overrun risk, if there's a very long token it won't fit. How to best handle that is left as an exercise. :)

¹ Why the temporary vector is called "number" when it is used to store two numbers and one string (the name) is beyond me.

Answer 2

Your fgets() call needs to specify 45 as the size or you oveflow the buffer when fgets writes the NULL terminator. That will set the "delims" string to be an empty string.

Also you are not returning any value even though the function declaration states it returns an int.

I don't know what the definition of your "struct student" is, but you may be overflowing buffers when using strcpy(). Also you decrement "recordCtr". Why? Why do you open the file for writing if you can't open it for writing? Why? If that fails too, you call fclose on a NULL pointer. I doubt that helps much.

I just noticed that you do not initialise "number". If you don't get three numbers on the first line, you then strcpy() from an uninitialised pointer. It probably has a NULL value so the program will segfault.

Also you have an array of size 3, but if the line you read has more than 3 comma separated fields you will overflow the array.

There are probably many other errors as well.

A lot of programmers just can't be bothered to do all the good coding practices like checking return values, initialising variables, and so on. They often end up with code like this. If you want to be a really good programmer, try and get in the habit of doing all these things, or at least always thinking about whether you need to or not.

There are so many potential bugs in this code. What happens if a line is more than 45 characters long? You don't strip newlines. You don't convert the strings to numbers (although number[1] appears to be a string data so why store it in an array called "numbers"?) or check that fgets actually returned any data or check how many pieces of data you get.

Answer 3

Let the buyer beware.

strtok may have some edge cases to worry about.

"one,two,three" will yield 3 tokens.

"one,,three" will yield 2 tokens.