Constructive criticism

[virtual008]

I'm learning C and can someone tell me if I'm doing file input right?

I know file and user input can be insecure because of buffer overflows.

#include<stdio.h>

int main(int argc,char *argv[])
{
    FILE *file;
    char c;
    int lines = 1;

    if(argc == 2)
    {
        file = fopen(argv[1],"r");

        if(file != NULL)
        {
            do
            {
                c = fgetc(file);

                if(c == '\n')
                {
                    lines ++;
                }
            }
            while(c != EOF);

            printf("%s has %d lines\n",argv[1],lines);

            return 0;
        }
        else
        {
            puts("Can't open file");

            return 1;
        }
    }
    else
    {
        puts("Only enter one file name");

        return 1;
    }
}

[Narue]

>char c;
c should be an int, because that's the type that fgetc returns. Specifically, EOF may not be handled properly and certain values may overflow a char.

You should also limit the scope of your variables as much as possible. I'd write it more like this:

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

int main ( int argc,char *argv[] )
{
  if ( argc == 2 ) {
    FILE *file = fopen ( argv[1], "r" );

    if ( file != NULL ) {
      int c;
      int lines = 1;

      do {
        c = fgetc ( file );

        if ( c == '\n' )
          lines ++;
      }
      while ( c != EOF );

      printf ( "%s has %d lines\n", argv[1], lines );
    }
    else {
      puts ( "Can't open file" );
      return EXIT_FAILURE;
    }
  }
  else
  {
    puts ( "Only enter one file name" );
    return EXIT_FAILURE;
  }

  return EXIT_SUCCESS;
}

Beyond that it looks like a good start. :)

>I know file and user input can be insecure because of buffer overflows.
Of course, that doesn't apply to your program as you aren't using a buffer. ;)

[virtual008]

Is the scope you use supported by c90 or is it a c99 feature?And thanks for the buffer over flow info so would a buffer be if I read the file to a string?

[Narue]

>Is the scope you use supported by c90 or is it a c99 feature?
You're thinking of the mixed declarations feature of C99, which is different. C90 supports declarations at the beginning of any block, not just the beginning of the function, while C99 supports declarations mixed with other statements.

>so would a buffer be if I read the file to a string?
Yes, in that case you have to consider lines that are longer than the string can hold. Just blindly copying characters into the string could overflow the memory, and that's what you need to look for.

[ssharish2005]

Perhaps, you could have a look at a fgets function, which actually reduces few lines of code, but still does what you wanted to do. Have a look at that function. This is just to give you some more clarity for your code.

ssharish

[Narue]

>you could have a look at a fgets function, which actually reduces few lines of code
Really? Let's look at the original:

do {
  c = fgetc ( file );

  if ( c == '\n' )
    lines ++;
} while ( c != EOF );

Six lines total, five lines with code. Now let's look at the same solution using fgets:

while ( fgets ( buffer, sizeof buffer, file ) ) != NULL ) {
  char *newline = strchr ( buffer, '\n' );

  if ( newline != NULL )
    ++lines;
}

Hmm, six lines total and five lines with code... Sure, you can play formatting tricks to make the code more compact, but the base solution is the same length. Or you can alter the organization of the original and have something more compact than with fgets.:

while ( ( c = fgetc ( file ) ) != EOF ) {
  if ( c == '\n' )
    ++lines;
}

>This is just to give you some more clarity for your code.
That's debatable. Personally I think it's more obvious what the original code is doing.

[ssharish2005]

Not really very sure why check if the line has a '\n' char in the buffer. It's obvious the fgets i gonna really until '\n' or EOF as per the specification of fgets.

This is what i thought when i posted.

while( fgets( buffer, sizeof buffer, fp ) != NULL )
               line++;

ssharish

[Narue]

>It's obvious the fgets i gonna really until '\n' or EOF as per the specification of fgets.
It may be obvious, but it's wrong because fgets is constrained by the size of the buffer you give it. Let's say your buffer is large enough to hold 10 characters and a null character. What happens if the line contains 20 characters?

The specification of fgets says that the buffer will be filled as usual, but because there's no newline, no newline will be stored. This way you can use fgets with multiple calls to read lines that are longer than the size of the buffer simply by checking for the presence of a newline character. If there's a newline, you've found the end of the line. If there's not a newline, you've only read a partial line.

[ArkM]

Some addition. As usually, you may get a drastic changes in effectiveness with setvbuf:

/* File i/o buffer size: the more the better (up to 64K or near) */
#define BSZ (16*1024)
...
FILE *file = fopen ( argv[1], "r" );
...
/* Before any input on the file make call: */
setvbuf(file,0,_IOFBF,BSZ);

Try and see elapsed times with large files, vary BSZ.

As usually, by default RTL allocates too small file buffers or does not select a proper buffer management discipline...