8
\$\begingroup\$

Very simple code, works fine. I'm mainly interested in the method I used to count lines. I thought about using fgetc, but I'm not even sure if it'd read the newline character, and I think it's slower too.

#include <stdio.h>
#include <stdlib.h>
#define MAX_SIZE 1000

int main(void){
    FILE *in_file;
    char line[MAX_SIZE];

    in_file = fopen("test", "r");
    if(in_file == NULL){
        fprintf(stderr, "Unable to open file");
        exit(EXIT_FAILURE);
    }

    int counter = 0; /*Number of lines*/

    while(fgets(line, sizeof(line), in_file) != NULL){
        counter++;
    }

    printf("Number of lines in the file is %i", counter);

    return 0;
}
\$\endgroup\$
7
\$\begingroup\$

As you say, it works fine. But I can nitpick.

The main flaw is that you could get a wrong count if any line is longer than 999 bytes. (In general, you should stress-test your code by cranking down the buffer size to ridiculously small numbers and checking whether you obtain the same results.)

The performance could be improved. Since fgets() has no way of knowing in advance where the newlines occur, it must read the file contents into a temporary buffer (which is invisible to you), then copy each line into the line buffer. Since only care about counting '\n' characters, you could read fixed-size blocks to avoid this internal copying.

It's awkward to read 1000 bytes at a time. You would be better off reading chunks that are aligned with the blocks on the disk. A better choice would be 1024.

You could use the perror() function to report why I/O operations failed. Technically, fgets() could fail too, so you should check for that.

It's a good habit to ensure that your fopen() is paired with fclose().

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

#define SIZE 1024

int main(void) {
    const char filename[] = "test";
    FILE *in_file;
    char buffer[SIZE + 1], lastchar = '\n';
    size_t bytes;
    int lines = 0;

    if (NULL == (in_file = fopen(filename, "r"))) {
        perror(filename);
        return EXIT_FAILURE;
    }

    while ((bytes = fread(buffer, 1, sizeof(buffer) - 1, in_file))) {
        lastchar = buffer[bytes - 1];
        for (char *c = buffer; (c = memchr(c, '\n', bytes - (c - buffer))); c++) {
            lines++;
        }
    }
    if (lastchar != '\n') {
        lines++;  /* Count the last line even if it lacks a newline */
    }
    if (ferror(in_file)) {
        perror(filename);
        fclose(in_file);
        return EXIT_FAILURE;
    }

    fclose(in_file);
    printf("Number of lines in the file is %i\n", lines);
}

The for loop above is a bit tricky for beginners, but should be acceptable for experienced C programmers. The loop

for (char *c = buffer; (c = memchr(c, '\n', bytes - (c - buffer))); c++)

means:

  • Search for the next newline, starting from the pointer c, in the remaining bytes - (c - buffer) bytes that have been read but not examined yet.
  • If a newline is found, make c point to the position just after it.
  • If no newline is found, then we're done with this chunk. Try reading more input then.

The extra set of parentheses around c = memchr(…) is an indication to the compiler and to other programmers that the = is indeed intentional, and is not supposed to be ==.

\$\endgroup\$
  • 2
    \$\begingroup\$ Should file "test" contain null characters, this will cause strchr() to miss the true end of the line. Recommend to use memchr() instead of strchr() - might be faster too. \$\endgroup\$ – chux - Reinstate Monica Feb 28 '17 at 5:39
  • \$\begingroup\$ +1 because I would have never had the idea to check for null pointers in such an elegant way as this line for (char *c = buffer; (c = strchr(c, '\n')); c++). Is this good practise though? I was confused at first because I didn't expect an assignment in the part where you usually just check a condition - or is this something you'd want to drop for clarity reasons? \$\endgroup\$ – lucidbrot Feb 28 '17 at 7:54
  • \$\begingroup\$ Thanks for the response. I don't think I entirely understand what you did. I have no issues up until to while loop. Starting from the loop: As long as EOF hasn't been reached: Add a null character (to make it a null terminated string?) Set lastchar to the character before \0, which should be \n until the last line. In the for loop: Create a char pointer, starting from element 0, then reset the pointer to the location of the \n character, which causes the loop to only perform its block once. Also, doesn't this still read the entire file into the array buffer? \$\endgroup\$ – user127566 Mar 1 '17 at 16:42
1
\$\begingroup\$

fgetc() does read newline characters. It would be a pretty useless function if it didn't!

If you benchmark versions of the code using fgetc() and fgets(), quite likely you will see no measurable difference. (Do this on a fairly big text file - e.g. 100Mb, and make sure you run both programs several times, otherwise the results will depend mostly on how your operating system caches the file in memory when you read it, not on your code!).

Both fgets() and fgetc() will read the file one buffer at a time, and (with luck) the buffer size will be the optimum for the file system you are reading, not a guessed number like 1000 or 1024.

In fact, if you switch on compiler optimization, fgetc() is likely to be in-line code which usually amounts to no more than the single expression *bufptr++, where bufptr is a pointer into the operating system's file buffer.

This type of code is often a good illustration of the principle that "premature optimization without benchmarking is the root of all evil". Use the library routines that allows the simplest logic, not the one that you guessed might run fastest.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy