while(infile){} reads last record twice

[SteveDee]

I am writing a simple program to read in date from a lotto winning numbers file. The data is structured like this:

01/03/1998 6 - 12 - 20 - 33 - 34 - 50
01/10/1996 13 - 22 - 34 - 40 - 44 - 49
etc.

No matter how many records there are, the following code reports one record too many and reads the last number on the last date 7 times.

vector <string> dates;  //to hold all the dates
	string thisdate;        //to hold the current date being read in
	vector <int> numbers;   //to hold all the numbers
	int thisnumber;         //to hold the number currently being read in
	int count=0;
	char dash;            //will be used to get rid of the dashes between numbers


	//read the lotto file, putting dates into 1 vector, and numbers into another

	while(infile){
		infile>>thisdate;
		dates.push_back(thisdate);
		for(int i=0;i!=5;i++){
			infile>>thisnumber;
			frequency[thisnumber-1]++;
			numbers.push_back(thisnumber);
			infile>>dash;
		}
		infile>>thisnumber;
		frequency[thisnumber-1]++;
		numbers.push_back(thisnumber);
		count++;
	}

	cout<<"total number of lotto drawings was "<<count<<endl;

I believe the problem is somehow with while(infile), ie, I dont believe I understand exactly how it works. Any help will be greatly appreciated.

[Ancient Dragon]

your while statement is incorrect

while( infile>>thisdate )
{
   // blabla
}

[Narue]

>I believe the problem is somehow with while(infile),
>ie, I dont believe I understand exactly how it works.
Your intuition is correct. while ( infile ) is using infile's implicit conversion to void* to check for an error state. If the conversion returns a null pointer, it means the stream is in an error state, otherwise it's fine. End-of-file counts as an error state, by the way. Your problem is with the order of operations. Here's what happens with a 2 line file:

1) Check the state of the stream -> good
2) Read a record successfully
3) Process the last successfully read record

4) Check the state of the stream -> good
5) Read a record successfully
6) Process the last successfully read record

7) Check the state of the stream -> good
8) Fail to read a record due to end-of-file
9) Process the last successfully read record

10) Check the state of the stream -> bad, terminate the loop

The bold part is your problem. What you're doing is checking the state of the stream before trying to read a record and using that to control your loop. The error state of the stream is only set after you've tried and failed to read a record. You need to read the record first.

Ancient Dragon's solution uses the result of reading a record to control the loop, which will work properly because if the read fails, the stream is put in an error state and the resulting conversion to void* will produce a null pointer all in one expression.

[SteveDee]

I see it all clearly now. Being that this is my first reply, I must ask if it is considered good manners to say thanks to both of you, or do I just mark as solved with no comment? Aslo, I am not sure it matters but the one thing I don't understand now is why it process the "last successfully read record" again. I would have thought that the failure to read would generate an error.

[Narue]

>I must ask if it is considered good manners to say thanks to
>both of you, or do I just mark as solved with no comment?
It's polite to say thanks, but not necessary. And you should only mark the thread as solved when you have no more relevant questions. ;)

>I would have thought that the failure to read would generate an error.
It does generate an error, but you don't do anything with that error immediately and the error doesn't halt operation by default. Assuming your file consists of complete records, the error occurs here:

infile>>thisdate;

And you do all of this before even looking at the state of the stream (which would tell you that an error occurred):

dates.push_back(thisdate);
for(int i=0;i!=5;i++){
	infile>>thisnumber;
	frequency[thisnumber-1]++;
	numbers.push_back(thisnumber);
	infile>>dash;
}
infile>>thisnumber;
frequency[thisnumber-1]++;
numbers.push_back(thisnumber);
count++;

When the stream is in an error state, it doesn't do anything. Any request for input will return immediately and the variables you're writing to retain their original values. That's why thisdate, thisnumber, and dash all keep the same values when trying to read thisdate fails. infile is in an error state after that and any request for input is effectively a no-op.

By "generate an error", I assume you mean throw an exception. The iostream library doesn't do that by default, but you can tell it to, which is another solution to the problem:

#include <fstream>
#include <iostream>

int main()
{
  std::ifstream in ( "test.txt" );
  int value;

  // Force an exception on end-of-file
  // Comment this out to see the error reappear
  in.exceptions ( std::ios::eofbit );

  try {
    while ( in ) {
      in>> value;
      std::cout<< value <<'\n';
    }
  } catch ( std::ios::failure ) {
    // Normal operation (bad practice)
  }
}

That's not recommended though, because thrown exceptions are expensive and should be avoided for normal operation. Only throw an exception in a truly exceptional situation: one that you don't expect to happen.