Counting the number of lines in a file

Question

I'm easing back into C after several months of using Python and Matlab. Can anyone give me some pointers on how to improve my code's efficiency and readability?

#include <stdio.h>

#define MAX_ARG 2

int count( char FNAME[] );

int main( int argc, char *argv[]) {
    if( argc > MAX_ARG ) {
        printf("Too many arguments ( %d ) supplied.\n", argc);
        return 0;
    }
    int FILE_LENGTH = 0;
    FILE_LENGTH = count( argv[1] );
    printf("The length of %s is: %d lines.\n", argv[1], FILE_LENGTH);
    return 0;
}

int count( char FNAME[] ) {
    FILE *fp;
    int CHR;
    int LINE_COUNT = 0;
    fp = fopen( FNAME,"r" );
    while( ( CHR = fgetc( fp ) ) != EOF ) {
        if( CHR == '\n' ) {
             LINE_COUNT++;
        }
    }
    fclose( fp );
    return LINE_COUNT;
}

Answer 1

The code starts off very well, with a check to make sure there are not too many arguments, but it doesn't do enough error checking. There could be too few arguments as well (no file name on the command line). Errors in the execution of the file should return a non-zero number and zero for success. Try using the constants EXIT_SUCCESS and EXIT_FAILURE that are defined in stdlib.h. There probably should be an error check to see if fopen() returns a valid file pointer or not.

Generally in C variables are named in lower case, constants and macros are named in all upper case. The constant MAX_ARG follows this standard, but the variables LINE_COUNT, FNAME, FILE_LENGTH and CHR are all opposite to the standard.

The function fgets() would be very useful to the program, and possibly enhance performance. fgets() retrieves a line of text from a file if the buffer supplied is big enough.

You could also input a sector at a time as @Maybe_Factor suggests, although it would probably be better to start testing with 4K, then 8K and up to 64K based on the operating systems blocking factor.

Answer 2

You could try reading the file in chunks into a buffer (start small (8 bytes), for testing, then make it 4096 bytes) and iterating through the buffer. It will be less readable, but has the advantage of minimising the overhead of reading the file.

Most of the overhead is in getting the disk to move to the right location to be read from, so reducing the number of times this happens should help speed up your program.

Answer 3

When counting lines in a file, code needs to consider 2 typical problems:

1) If the last line does not end with a '\n', does it count as a line? IMO, it should. Solution: increment line count when a character is read after a '\n' or is the first character.

2) Might the file contain embedded null characters? The occurrence of these prevents proper use of fgets(). In this case, OP does not use fgets(), but the question is still of consideration.

Code should not assume a file will consistent of INT_MAX lines or less. Recommend unsigned long long or uintmax_t.

unsigned long long line_count = 0;
int previous = '\n';
int chr;
while((chr = fgetc(fp)) != EOF) {
  if (previous == '\n') line_count++;
  previous = chr;
}

Answer 4

minor thoughts.

In general n v nice, but stop USING ALL CAPS for variables (as others have said)

Some of your layout is non-standard. Eg

int count( char FNAME[] );

Although correct syntax is unusual. Most would expect

int count(char FNAME[]);

and the truly idiomatic for a string is

int count(char *fileName);

This is slightly old fashioned (comes from the days when all local variables had to be declared at start of scope).

   int FILE_LENGTH = 0;
    FILE_LENGTH = count( argv[1] ); 

Instead do

int fileLength = count(argv[1]);

Ideas for improvement

• allow a list of file names
• return a simple count instead of a human readable sentence. This allows you to pipe the output into another command.