-3
\$\begingroup\$

This is a program I made for practice in file handling in C++. The purpose of the program is to write three nos 1 2 and 3 into a file abc.dat and then read it and display it on the monitor. When I check the output I get 123123 instead of just 123. I don't understand why this occurs. When I uncomment the line //if(input.eof()) break; however this error is resolved and I get 123. Why is this so? The break statement seems purely unnecessary to me since the same condition is already specified in the while loop. Why does this statement influence the output I get?

#include <iostream.h>
#include <fstream.h>
#include <conio.h>

int main()
{
clrscr();
int first=1,second=2,third=3;
ofstream add ;
add.open("abc.dat",ios::out) ;
add << first << second << third;
add.close();
ifstream input ;
input.open("abc.dat",ios::in);
int n;
while(!input.eof())
{
input >> n ;
//if(input.eof()) break;
cout<< n ;
}
input.close();
return 0;
}
\$\endgroup\$
1

1 Answer 1

2
\$\begingroup\$

The question you asked is really a Stack Overflow question, not a Code Review question. It will be answered, but in the course of a code review.

#include <iostream.h>
#include <fstream.h>

These are not C++ headers, and AFAIK never have been. What you want is:

#include <iostream>
#include <fstream>

Also you don't use namespace std anywhere in your code, probably because of using the headers you do.

#include <conio.h>

clrscr();

You should know that this is an unportable header. Given that you only use a single function from it - and that function doesn't really serve a useful purpose - consider whether it's necessary.

int first=1,second=2,third=3;

Don't declare multiple variables on a single line.

Don't declare variables before they're needed. You don't need these variables until 3 lines later... after you open the output file.

ofstream add ;
add.open("abc.dat",ios::out) ;

That should be std::ofstream.

There is no need to explicitly state std::ios::out in the open() command. It's an **o**stream; it knows to open in out mode.

Did you know that you can open the file directly in std::ofstream's constructor? There's no reason not to, unless you were doing something like setting exceptions first.

add.close();

If you write your code with better structure, you don't need to explicitly close files. File streams close automatically.

To get this effect, though, you need to add some more structure to your code. You could group all the output operations in one set of braces and all the input operations in another. But even better would be to create two functions - one for doing the output, the other for doing the input. For example, the output function might look like this:

void write_numbers()
{
    std::ofstream add{"abc.dat"};

    int first = 1;
    int second = 2;
    int third = 3;

    add << first << second << third;

    // File closes automatically.
}

The same pattern applies to the number-reading part of the program.

ifstream input ;
input.open("abc.dat",ios::in);

Once again, you need std::, you can do the open with the constructor - you don't need two lines - and you don't need std::ios::in because it's an ifstream.

while(!input.eof())
{
input >> n ;
if(input.eof()) break;
cout<< n ;
}

The reason you need to repeat the test is that the input read doesn't trigger eof until it actually hits the end of the file.

The first time through the loop, eof() is obviously false. The line input >> n; reads in 123... and then stops, seeing as there are no more numbers. The if test is false because you haven't hit the end of the file yet. The output works as expected.

The second time through the loop, eof() is still false because you haven't actually hit the end of the file yet. Then you try to read in another number... and that is when you hit the end of the file. Now the if detects eof(), and the loop ends.

The proper way to do this pattern looks like this:

while (input >> n)
{
    std::cout << n;
}

The way this loop works is that the first time through, you read in the number and check the stream state in the while condition. The result will be okay, because you successfully read a number. Then you print it.

The second time through the loop, you try to read in another number... and immediately hit EOF, which means the input failed. So the while test is false, and the loop ends.

input.close();
return 0;

Neither of these two lines is necessary. The file stream will close itself automatically, and you don't need return 0; in main, because it's added automatically.

Summary

  • Use standard C++ headers.
  • Use namespace std.
  • Avoid unportable stuff... especially pointless unportable stuff.
  • Don't declare multiple variables on a single line, and don't declare variables until you need them.
  • Use functions to break up the stages of your program.
  • Take advantage of automatic behaviours of library classes like file streams.
  • It's usually easier to test whether an input succeeded, not whether the stream is at EOF or in a fail state.
\$\endgroup\$
4
  • \$\begingroup\$ Thank you! Actually about the <iostream.h>, etc. that is what my textbook and the version of Turbo C++ I'm using accept..I think I'm using an older version. I actually didn't know stack overflow was the website I was supposed to use, isn't code review for reviewing code? Also I'm so sorry to trouble you, but is there any way the program can be made to take 1, 2 and 3 separately? Why does it take them at one go? \$\endgroup\$
    – Hema
    Jun 7, 2018 at 6:33
  • 2
    \$\begingroup\$ CR is a site for reviewing working code. Broken code belongs on SO. \$\endgroup\$
    – yuri
    Jun 7, 2018 at 6:35
  • \$\begingroup\$ @yuri is there a way I can transfer this to stack overflow? \$\endgroup\$
    – Hema
    Jun 7, 2018 at 6:36
  • 4
    \$\begingroup\$ You should not answer off-topic questions, as this defeats the purpose of having specialized SE communities. \$\endgroup\$
    – Daniel
    Jun 7, 2018 at 6:43

Not the answer you're looking for? Browse other questions tagged or ask your own question.