There are a number of small but important issue with your code. The primary one is a failure to validate your file is actually open for reading. If you do not validate fopen succeeds, you have no way of knowing if you are invoking Undefined Behavior on the next call that attempts to read from an invalid fin pointer.
You have been pointed to the link explaining Why is “while ( !feof (file) )” always wrong? Validating the return of fgets is all that is required.
Next, while it is fine to pass a pointer to rows as a parameter to your function, you are not updating it correctly. In get_num_lines you attempt update with:
rows++; /* this is wrong. this increments the address! (not the value) */
Since you passed a pointer, you must increment the value stored at that address, not the address itself, e.g.
(*rows)++; /* note the use of (..) for correct C-operator precedence */
That brings up a more practical question, "Why pass a pointer at all?" Why not just make use of a meaningful return of get_num_lines and simply return the number of lines to the caller?, e.g. size_t get_num_lines (FILE *fin)
note: the general practice is to open and validate that the file is open for reading in the calling function (main() here) and pass a FILE * pointer as a parameter rather than a filename. Passing a filename and handling it all in the function is not wrong, it's just not the general approach.
However, you cannot simply call fgets to count the number of lines in a file. You must validate that the line fit in your buffer (e.g. that you read an entire line and just not the first 254 chars of a longer line) before you increment the line count. To do that you need to check the length of the line read by fgets and validate that the last character read was the '\n'.
There is one more (unfortunately common) problem that will cause your line-count to be too few by 1 if the file has a non-POSIX end of file (meaning it is missing the final '\n'). This is a side-effect of properly validating the final character is a '\n' -- required for the proper operation of your count function. If the file does not have a final '\n' your check for it will fail causing the last line to go uncounted. Thankfully this is handled simply by setting a flag indicating that no end of line was read, and then checking if the flag is set after leaving the fgets read loop.
Putting those pieces altogether, a function taking an open FILE* pointer, reading and returning the number of lines present could be:
size_t fgets_nlines (FILE *fp)
{
int noeof = 0;
size_t n = 0;
char buf[BUF_SIZE] = "";
while (fgets (buf, BUF_SIZE, fp)) { /* read until EOF */
size_t len = strlen (buf); /* get buf length */
if (len && buf[len-1] != '\n') { /* if not complete line */
noeof = 1; /* set flag no EOL found */
continue; /* read until all chars in line are read */
}
noeof = 0;
n++;
}
if (noeof) /* handle non-POSIX EOF (add 1 to count) */
n++;
return n;
}
A second line oriented function provided by POSIX that does not require the end of file check is POSIX getline. It also has the benefit of allocating sufficient storage -- regardless of the length of the line. (which can also be viewed as a drawback as well). You can do the same thing with getline with something similar to:
size_t getline_nlines (FILE *fp)
{
size_t lines = 0, n = 0;
char *buf = NULL;
while (getline (&buf, &n, fp) != -1)
lines++;
free (buf);
return lines;
}
A short example program using either (you have to adjust the function names) could be written as follows. It takes the filename to read from as the first argument for the program (or reads from stdin by default if no argument is given) It provides output similar to wc -l on Linux, appending the filename read as part of the line-count output if the name was provided as an argument, or simply outputs the line-count alone if reading from stdin, e.g.
#include <stdio.h>
#include <stdlib.h> /* for free if using getline */
#include <string.h>
#ifndef BUF_SIZE /* fgets buffer size */
#define BUF_SIZE 8192
#endif
size_t fgets_nlines (FILE *fp); /* comment/uncomment as required */
// size_t getline_nlines (FILE *fp);
int main (int argc, char **argv) {
size_t nlines = 0;
FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;
if (!fp) { /* validate file open for reading */
perror ("file open failed.");
return 1;
}
nlines = fgets_nlines (fp);
// nlines = getline_nlines (fp); /* same note, comment/uncomment */
if (nlines) {
if (argc > 1)
printf ("%zu %s\n", nlines, argv[1]);
else
printf ("%zu\n", nlines);
}
if (fp != stdin) fclose (fp); /* close file if not stdin */
return 0;
}
Look things over, think about the issues involved, and the difference between how fgets and getline handle a non-POSIX EOF and why. Let me know if you have any further questions.
