c++ EOF running one too many times?

Question

This is my first time using EOF and/or files, and I am having an issue where my code hangs, which I believe is because my EOF is looping one too many times.

I am imputing from a file, and dynamically creating objects that way, and it hangs once the file is run through.

        while( !studentFile.eof() )
    {
        cout << "38\n";
        Student * temp = new Student();
        (*temp).input( studentFile );

        (*sdb).insert( (*temp) );           
    }

This chunk of code is the code in question. The cout >> "38\n"; is the line number and the reason I believe it is hanging from looping one too many times.

The file only contains 4 student's worth of data, yet 38 appears 5 times, which is the reason I believe it is looping one too many times; Once it gets the last bit of data, it doesn't seem to register that the file has ended, and loops in again, but there is no data to input so my code hangs.

How do I fix this? Is my logic correct?

Thank you.

Answer 1

This is because the EOF flag is only set after you try to read and get no data. So it would go

Test for EOF -> No EOF
Try to read one line -> Good, read first line
Test for EOF -> No EOF
Try to read one line -> Good, read second line
Test for EOF -> No EOF
Try to read one line -> Good, read third line
Test for EOF -> No EOF
Try to read one line -> Good, read fourth line
Test for EOF -> No EOF
Try to read one line -> EOF

But by the Try to read one line -> EOF, you're already in the body of the while on the fifth iteration, which is why you're seeing the loop run 5 times. So you need to read before you check for EOF.

Answer 2

Others have already pointed out the details of the problem you've noticed.

You should realize, however, that there are more problems you haven't noticed yet. One is a fairly obvious memory leak. Every iteration of the loop executes: Student * temp = new Student();, but you never execute a matching delete.

C++ makes memory management much simpler than some other languages (e.g., Java), which require you to new every object you use. In C++, you can just define an object and use it:

Student temp;

temp.input(studentFile);

This simplifies the code and eliminates the memory leak -- your Student object will be automatically destroyed at the end of each iteration, and a (conceptually) new/different one created at the beginning of the next iteration.

Although it's not really a bug per se, even that can still be simplified quite a bit. Since whatever sdb points at apparently has an insert member function, you can use it like a standard container (which it may actually be, though it doesn't really matter either way). To neaten up the code, start by writing an extraction operator for a Student:

std::istream &operator>>(std::istream &is, Student &s) {
    s.input(is);
    return is;
}

Then you can just copy data from the stream to the collection:

std::copy(std::istream_iterator<Student>(studentFile),
          std::istream_iterator<Student>(),
          std::inserter(*sdf));

Note that this automates correct handling of EOF, so you don't have to deal with problems like you started with at all (even if you wanted to cause them, it wouldn't be easy).

Answer 3

You need to check the stream status bits immediately after performing an operation on a stream. You don't show the code, but it would appear that (*temp).input(studentFile) is doing the reading from the stream. Call eof() (or other status check) after doing the read but before processing with the data you (attempted to) read.