1

I am suppose to pass stream, which is a pointer, by reference. So I am passing this as a pointer to a pointer. Can someone please verify my code?

    int main(int argc, char** argv)
{
    FILE *stream;

    printf("LINES: %d\n",scan(stream));
}

int scan(FILE *(*stream))
{
    stream = fopen("names.txt", "r");

    int ch = 0, lines=0;

    while (!feof(*stream))
    {
        ch = fgetc(*stream);
        if (ch == '\n')
        {
            lines++;
        }
    }
    fclose(*stream);
    return lines;
}

No output received.

user3337714
  • 663
  • 1
  • 7
  • 25
  • 1
    [while-feof-file-is-always-wrong](http://stackoverflow.com/a/5432517/3386109) – user3386109 Mar 02 '15 at 07:10
  • 1
    _Can someone please verify my code?_ The compiler will verify your code. – M Oehm Mar 02 '15 at 07:13
  • @user3386109 It's not wrong. I have test and it works. – user3337714 Mar 03 '15 at 02:39
  • @MOehm I did compile and thus I stated _No output received_. I am learning. – user3337714 Mar 03 '15 at 02:40
  • Appears to work it does, but not for the reasons you believe. There's a difference between code that is correct, and code that works for the wrong reasons. `while ( (ch = fgetc(*stream)) != EOF )` is the correct way to write that `while` loop. – user3386109 Mar 03 '15 at 03:02
  • @user3386109 As i mentioned I am learning, I picked up this format from the answer given by _Dominic Rodger_ on page [link](http://stackoverflow.com/questions/2162497/efficiently-counting-the-number-of-lines-of-a-text-file-200mb) – user3337714 Mar 03 '15 at 03:06
  • Ahh, but notice all of the dollar signs in the code. That's not C code, it's PHP. Check the tags in the question. – user3386109 Mar 03 '15 at 03:11
  • @user3386109 I will try to be more careful. Can you help me in another question please [link](http://stackoverflow.com/questions/28823218/rewind-a-text-file-and-populate-dynamic-array-stream-passed-by-value) – user3337714 Mar 03 '15 at 03:14

3 Answers3

2

Use

int scan(FILE **stream) //no need for brackets
{
    *stream = fopen("names.txt", "r"); //* is for dereferencing

    if(*stream==NULL) // Checking the return value of fopen
    {
        printf("An error occured when opening 'names.txt'");
        return -1;
    }

    int ch = 0, lines=0;

    while ((ch = fgetc(*stream))!=EOF) //while(!feof) is wrong
    {

        if (ch == '\n')
        {
            lines++;
        }
    }
    fclose(*stream); // Close the FILE stream after use
    return lines;
}

int main(void)
{
    FILE *stream;

    printf("LINES: %d\n",scan(&stream)); //Pass address of `stream`. The address is of type `FILE**`
}
Spikatrix
  • 20,225
  • 7
  • 37
  • 83
  • Yes, but what good is it to pass a pointer to file handle if you don't use the created handle outside the function? You (and the OP) should also `fclose´ the file. – M Oehm Mar 02 '15 at 07:20
  • 1
    `while ( (ch = fgetc(*stream)) != EOF )` – user3386109 Mar 02 '15 at 07:22
  • @MOehm ,Maybe the OP has some requirement. And thanks for reminding me about the `fclose`. I've added it. – Spikatrix Mar 02 '15 at 07:53
  • @CoolGuy: Yes, of course, there might be a special requirement that we don't know of. Now that the OP has inserted `fclose` it is clear that there is simply a misconception of what passing pointers to stuff from outside is for. Your answer answers the question, but in my opinion, the question was a bit wrong-headed in the first place. Never mind. – M Oehm Mar 02 '15 at 07:59
2

Your code has design issues. What exactly do you want to achieve?

If you just want to count the lines, make the FILE * local to your function:

int count_lines(const char *filename)
{
    FILE *stream = fopen(filename, "r");

    int lines = 0;

    while (1) {
        int c = fgetc(stream);

        if (c == EOF) break;
        if (c == '\n') lines++;
    }
    fclose(stream);

    return lines;
}

If you want to do a regular file operation (read, write, seek, rewind etc.) to a file that has already been opened with fopen, just pass the handle as FILE *:

int fget_non_space(FILE *stream)
{
    int c;

    do {
        c = fgetc(stream);
    } while (isspace(c));

    return c;
}

In that case, both fopen and fclose are called outside this function. (You don't call fclose in your program, which you should, even if the operating system makes sure to close the file automatically after exiting.)

Passing a pointer to the file handle, FILE **, makes sense only if you want to change that file handle itself in the function, for example by calling fopen:

int fopen_to_read(FILE **FILE pstream, const char *fn) 
{
    *pstream = fopen(fn, "r");
    return (*pstream != NULL) ? 0 : -1;        
}

Even then, it would be better to return the file handle, as fopen does.

Your example code leaves the open filehandle accessible in main, but you don't do anything with it, you don't even close it. Is that what you want? I doubt it.

M Oehm
  • 28,726
  • 3
  • 31
  • 42
  • Thank you so much for the detailed explanation. It has helped me understand the whole logic/concept. But simply, I wanted to pass 'stream' by reference. I did forget to close the file (apology), I have corrected the original code in the question. – user3337714 Mar 02 '15 at 07:46
  • Yes, but you are aware that in your example, there isn't any point in passing the file pointer, because it is never used outside `scan`. That design is even dangerous, because before calling `scan`, `stream` is uninitialised and after callig `scan`, it refers to a close file. Still, you _can_ access `stream` in both cases, where it is invalid. The proper design would be to "encapsulate" the file handle and restrict its scope to its use. – M Oehm Mar 02 '15 at 07:56
  • I will try to learn better. It has been difficult for me to learn C, But I am trying to understand. Can you assist me on another problem [link](http://stackoverflow.com/questions/28823218/load-text-file-in-struct) – user3337714 Mar 03 '15 at 04:01
1

Replace

stream = fopen("names.txt", "r");

with

*stream = fopen("names.txt", "r");

Also

printf("LINES: %d\n",scan(stream));

with

printf("LINES: %d\n",scan(&stream));
Vagish
  • 2,520
  • 19
  • 32