0

I have a bug with my code (the code at the end of the question). The purpose of my C++ executable is to read a file that contains numbers, copy it in a std::vector and then just print the contents in the stdout? Where is the problem? (atoi?)

I have a simple text file that contains the following numbers (each line has one number)

mini01:algorithms ios$ cat numbers.txt 1 2 3 4 5

When I execute the program I receive one more line:

mini01:algorithms ios$ ./a.out 1 2 3 4 5 0

Why I get the 6th line in the stdout? 

#include <iostream> #include <string> #include <fstream> #include <vector> using namespace std; void algorithm(std::vector<int>& v) { for(int i=0; i < v.size(); i++) { cout << v[i] << endl; } } int main(int argc, char **argv) { string line; std::vector<int> vector1; ifstream myfile("numbers.txt"); if ( myfile.is_open()) { while( myfile.good() ) { getline(myfile, line); vector1.push_back(atoi(line.c_str())); } myfile.close(); } else { cout << "Unable to open file" << endl; } algorithm(vector1); return 0; }
Improve this question
6
  • 2
    We see this error countless times. If you found the answers below helpful I'd appreciate if you'd explain why you thought while (myfile.good()) was correct. Did you see it in a book, did someone tell you to use it, did it just feel right? etc. Commented Apr 10, 2013 at 13:16
  • 1
    @john I'll second that. We're constantly seeing this (and while ( ! myfile.eof() ), but where do people learn this? We've got to find out, and do something to warn about it. Commented Apr 10, 2013 at 13:20
  • 1
    @john Good point. Maybe we should tackle this from the source. Commented Apr 10, 2013 at 13:23
  • @john thanks for your comment. Actually I was looking for a code snippet to read from a file, instead of writing the code from scratch. I had to do to a quick patch. I googled "C++ input output" or something similar. I clicked at cplusplus.com/doc/tutorial/files. If you go to the middle of the html file, you with find a section "Text files". The second example is wrong, and it is probably the root cause of the countless appearances of similar threads in the context of SO. Even cplusplus.com can go wrong. Bottom line: "Code it yourself,even if it takes some more time". Don't paste! Commented Apr 11, 2013 at 7:46
  • 1
    @cateof Actually they do, I'll think I try suggesting a correction and see what happens. Commented Apr 15, 2013 at 9:32

4 Answers 4

Reset to default
6

You should not use while (myfile.good()), as it will loop once to many.

Instead use

while (getline(...))

The reason you can't use the flags to check for looping, is that they don't get set until after an input/output operation notices the problem (error or end-of-file).

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

And the reason getline doesn't notice the end of file here is because it stops as soon as it has extracted the very last \n character.
Also, he doesn't verify that the getline actually worked before using the results, and since he's using atoi, he can't verify anything with regards to the syntax of what he has read. (The first issue is related to your point, and solved by your solution. The second remains a problem in his code.)
2

Don't use good() as the condition of your extraction loop. It does not accurately indicate whether the next read will succeed or not. Move your call to getline into the condition:

while(getline(myfile, line)) { vector1.push_back(atoi(line.c_str())); }

The reason it is failing in this particular case is because text files typically have an \n at the end of the file (that is not shown by text editors). When the last line is read, this \n is extracted from the stream. Yes, that may be the very last character in the file, but getline doesn't care to look any further than the \n it has extracted. It's done. It does not set the EOF flag or do anything else to cause good() to return false.

So at the next iteration, good() is still true, the loop continues and getline attempts to extract from the file. However, now there's nothing left to extract and you just get line set to an empty string. This then gets converted to an int and pushed into the vector1, giving you the extra value.

In fact, the only robust way to check if there is a problem with extraction is to check the stream's status bits after extracting. The easiest way to do this is to make the extraction itself the condition.

Improve this answer

4 Comments

FWIW: the standard actually requires that text files end with a '\n'. (What happens if they don't is either undefined or unspecified, and all of the implementations for Windows or Unix behave reasonably.)
@JamesKanze I think you've mentioned this before (or somebody else did), but I've never seen the quote. Any idea where it is?
In the C standard:-). C++ defines IO in terms of C IO, and C has this restriction (§7.19.2/2 in C99, but it was present in C90 as well). And it's "implementation defined" (not undefined or unspecified) in C99. I can't imagine a missing final '\n' causing any problems when files are byte streams (as in Windows or Unix), but on some file systems I've seen, I could imagine it causing the last line (the one without the '\n') to disappear.
BTW, that's an excellent explination of what is actually happening (doubtlessly, since it's very, very unlikely that the OP is running on a mainframe or anything else with an exotic file system).
1

You read one too many lines, since the condition while is false AFTER you had a "bad read".

Improve this answer

Comments

1

Welcome to the wonderful world of C++. Before we go to the bug first, I would advise you to drop the std:: namespace resolution before defining or declaring a vector as you already have

using namespace::std;

A second advise would be to use the pre increment operator ++i instead of i++ wherever feasible. You can see more details on that here.

Coming to your problem in itself, the issue is an empty new line being read at the end of file. A simple way to avoid this would be to check the length of line before using it. 

getline(myfile, line); if (line.size()) { vector1.push_back(atoi(line.c_str())); }

This would enable your program now to read a file interspersed with empty lines. To be further foolproof you can check the line read for presence of any non numeric characters before using atoi on it. However the best solution as mentioned would be use to read the line read to the loop evaluation.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.