Why is my program printing the last record of a file twice?

Question

I created a simple bank application program to ask a user whether they want to add a bank record to a file or show all the records available. Both these functions are facilitated by write_rec() and read_rec() respectively. But when the function read_rec() is applied, while it does print all the records available in the file(a single file is used to store all the records), for some reason it prints the last record in the file two times instead of just once. It's very frustrating to see everything works so well then this problem pops up out of nowhere. I tried to see where the issue is but for the life of me I just can't find it. Can you guys please help me with this one?

Here's the code:

#include<iostream>
#include<fstream>
#include<cstdlib>

using namespace std;

class account_query
{
    char account_number[20];
    char firstName[10];
    char lastName[10];
    float total_Balance;
public:
    void set_data();
    void show_data();
    void write_rec();
    void read_rec();
};

void account_query::set_data()
{
    cout<<"\nEnter Account Number: ";
    cin>>account_number;
    cout<<"Enter First Name: ";
    cin>>firstName;
    cout<<"Enter Last Name: ";
    cin>>lastName;
    cout<<"Enter Balance: ";
    cin>>total_Balance;
    cout<<endl;
}

void account_query::show_data()
{
    cout<<"Account Number: "<<account_number<<endl;
    cout<<"First Name: "<<firstName<<endl;
    cout<<"Last Name: "<<lastName<<endl;
    cout<<"Current Balance: Rs.  "<<total_Balance<<endl;
    cout<<"-------------------------------"<<endl;
}

void account_query::write_rec()
{
    ofstream outfile;
    outfile.open("D:/rec.bin", ios::binary|ios::in|ios::out|ios::app);
    set_data();
    outfile.write(reinterpret_cast<char *>(this), sizeof(*this));
    outfile.close();
}

void account_query::read_rec()
{
    ifstream outfile;
    outfile.open("D:/rec.bin", ios::binary);
    if(!outfile.is_open())
    {
        cout << "Error! File not found!" << endl;
        return;
    }
    cout << "\n****Data from file****" << endl;
    while(outfile.good())
    {
        outfile.read(reinterpret_cast<char *>(this), sizeof(*this));
        show_data();
    }
    outfile.close();
}

int main()
{
    account_query A;
    int choice;
    cout << "***Account Information System***" << endl;
    while(true)
    {
        cout << "Select one option below";
        cout << "\n\t1-->Add record to file";
        cout << "\n\t2-->Show record from file";
        cout << "\n\t3-->Quit";
        cout << "\nEnter you chice: ";
        cin >> choice;
        switch(choice)
        {
            case 1:
                A.write_rec();
                break;
            case 2:
                A.read_rec();
                break;
            case 3:
                exit(0);
                break;
            default:
                cout << "\nEnter correct choice";
                exit(0);
        }
    }
    system("pause");
    return 0;
}

And this is the result I get when I try to print the record on the console:

enter image description here

Please help me

`while(outfile.good())` is incorrect. Test the actual read operation for success or failure and only use the data if it was successful. — Retired Ninja, Jul 22 '21 at 19:40
These are not the exact code you're using, but the concept is what's important. https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-i-e-while-stream-eof-cons https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong — Retired Ninja, Jul 22 '21 at 19:42
That's because [using eof in a loop condition is always wrong](https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-i-e-while-stream-eof-cons). You're using `good()` instead of `eof()`, but this is a logically identical bug. Same bug. Same fix. — Sam Varshavchik, Jul 22 '21 at 19:45
You don't check if `read` failed but call `show_data` even if it did. You are expecting `good()` to predict whether a future operation will succeed or fail, and it can't do that. — David Schwartz, Jul 22 '21 at 19:55
Somewhat unrelated, but consider splitting out the data record into its own type and have another class to do the reading and writing (or just simply top-level functions). It's strange to constantly replace the contents of your record while you're reading. It's a very stateful approach that will lead to bugs and strange behavior in the long run. — beta, Jul 22 '21 at 20:19

Andreas Wenzel · Accepted Answer · 2021-07-22T20:54:24.263

As already pointed out in the comments section, the problem is that the line

while(outfile.good())

will only check whether the stream extraction has already failed. It will not tell you whether the next stream extraction operation will fail or not. It is unable to provide this information.

Therefore, you must check the state of the stream after the attempted stream extraction operation, to see whether it succeeded or not. So you must check the stream state after this line:

outfile.read(reinterpret_cast<char *>(this), sizeof(*this));

It should be checked before you call show_data, because you don't want to call show_data if the stream extraction failed.

The simplest fix would be to change the lines

while(outfile.good())
{
    outfile.read(reinterpret_cast<char *>(this), sizeof(*this));
    show_data();
}

to the following:

while( outfile.read(reinterpret_cast<char *>(this), sizeof(*this) ) )
{
    show_data();
}

This will work because istream::read will return a reference to the stream object outfile and writing

while ( outfile )

is equivalent to

while ( !outfile.fail() )

due to istream::operator bool being called.

Why is my program printing the last record of a file twice?

1 Answers1