My program continously loop after reading nodes into linked list and displays only one node repeatedly

Question

I created a Linked list in a C++ program that writes the nodes to a binary file and read them back to the linked list. However when displaying back the list the program continuously displays only one node (continuous loop).

Here's the code:

 #ifndef ACCOUNT_H
    #define ACCOUNT_H

    class Account{

private:
    double balance;
    unsigned int accountNumber;
    Account *next;
public:
    Account(){
        balance=0.0;
        accountNumber=0;
        next=NULL;
    }// primary constructor

    Account(unsigned int acc, double bal){
        accountNumber=acc;
        balance=bal;
        next=NULL;
    }// primary constructor

    Account::~Account(){
        //delete next;
    }// destructor


    void setBalance(double b){
        balance=b;
    }// end of setBalance


    double getBalance(){
        return balance;
    }

    void setAcc(unsigned int acc){
        accountNumber=acc;
    }

    unsigned int getAccNum(){
        return accountNumber;
    }

    //  links
    void setNextAccount(Account *nAcc){
        next=nAcc;
        //delete nAcc;
    }// Mutator for next

    Account *getNextAccount(){
        //Account *temp=next;
        return next;
    }
};
#endif

Linked list header file:

#ifndef ACCOUNTLIST_H
#define ACCOUNTLIST_H

class AccountList{
private :
    Account *head;
    //Account * temp;
    //Account *current;
public:
    AccountList(){
        head=NULL;
        //temp=NULL;
        //current=NULL;
        //current=NULL;
    }// end of default constructor

    AccountList::~AccountList(){
        /*Account *temp;
        while (head!= NULL)
        {
            temp = head;
            head = head->getNextAccount();
            delete temp;
        } */
        delete head;
        //delete current;
    }// destructor for list

    void addNode(Account *h){
        //Account *temp;
        Account *current;
        //temp=h;
        //temp->setNextAccount(NULL);
        if(head==NULL){
            head=h;
        }
        else{
            current=head;
            while((current->getNextAccount())!=NULL){
                current= current->getNextAccount();
            }
            current->setNextAccount(h);
        }
        //delete current;
    }// mutator for head

    void displayAll(){
        Account *temp=head;
        while ((temp!=NULL) && (temp->getAccNum()!=0)){         
            cout << "Account number: " <<temp->getAccNum() << " has a balnce of: " << temp->getBalance() <<endl;
            temp=temp->getNextAccount();
        }

        delete temp;
    }

    void displayNode(int id){
        Account *temp= head;
        while (temp !=NULL){

            if (temp->getAccNum()==id){
                //temp->display();
                cout << "Account Number : " << temp->getAccNum() <<"has a balance of " << temp->getBalance() <<endl;
                delete temp;
                return;
            }
            temp= temp->getNextAccount();// gets next node in the list
        }
        cout << "Employer was not found" << endl;
    }// end of displayNode


    Account* getHead(){
        return head;
    }

    void removeAll(){
        Account *temp;
        while (head!=NULL){
            temp=head;
            head= head->getNextAccount();
            delete temp;
        }
    }// end of method removeAll

};

#endif

main driver:

#include <iostream>
#include <fstream>
#include "Account.h"
#Include "AccountList"
//#include <cstdlib>
#include <stdlib.h>
using namespace std;

void main(){
    Account *acc1=new Account(1, 546.34); // create object and initialize attributes 
    Account *acc2=new Account(2,7896.34);
    Account *acc3=new Account();
    AccountList *list1= new AccountList();
    AccountList *list2= new AccountList();

    // add nodes to linked list
    list1->addNode(acc1);
    list1->addNode(acc2);
    //cout <<"Hello"<<endl;

    // file operation
    ofstream outAccount("account.dat", ios::out|ios::binary);

    // checks if ofstream could open file
    if(!outAccount){
        cerr<<"File could not be open" << endl;
        exit(1);
    }   
    acc3=list1->getHead();
    while( acc3!=NULL){
        outAccount.write(reinterpret_cast < const char*>(&acc3), sizeof(Account));
        acc3=acc3->getNextAccount();
        //outAccount.write(reinterpret_cast < const char*>(&acc2), `sizeof(Account));`
    }
    //cout <<"Hello"<<endl;
        outAccount.close();


    // read and display contents of file
    ifstream inAccount("account.dat", ios::in|ios::binary);
    if(!inAccount){
        cerr <<"File could not be openned for reading from file" << endl;
        exit(1);
    }
    //Account *accTemp=new Account();
    while(inAccount && !inAccount.eof()){

        inAccount.read(reinterpret_cast < char* >(&acc3), sizeof(Account));
        list2->addNode(acc3);

        //cout <<"Account Number : " << acc3->getAccNum()<< "has a balance of: " `<< acc3->getBalance() <<endl;`
    }

    inAccount.close();
    cout <<"Hello"<<endl;

    list2->displayAll();
    system("PAUSE");

    system("PAUSE");
}// end of main

Answer 1

There are numerous problems with your code, but I'll try to cover them all without simply dumping a revised code listing and hopefully you can work through the problems based on my descriptions.

First of all, there's no need to reinvent the linked list "wheel" as a std::list will give you everything you're trying to do with your AccountList class, but to do so you'd have to get familiar with iterators. Iterators are essentially the same thing as pointers, but I admit they can be confusing to someone with a C background. In any case, the remainder of my discussion assumes you continue with your AccountList class instead of using std::list.

Second of all, in your Account and AccountList constructors, you should be using initializer lists to initialize your member variables, since there's nothing about the variables that requires any computation or complex logic. E.g. do this:

Account()
 : balance(0.0), accountNumber(0), next(NULL) {}

Now on to the actual bugs:

When you do your outAccount.write(), you're writing the entire contents of acc3, including its pointers. Those pointers are valid only at that moment, and only for 'list1'. The next time you run your program, the system may have allocated something else at those addresses, and those pointers will not contain 'Account' objects much less the same ones as before. This is important to understand as you move down to the inAccount.read(), where you're reading in the old contents of the Account objects, along with the old pointer values. At this point those addresses aren't valid anymore, or at least don't apply to 'list2'. THIS is important to realize when you call list2->addNode(acc3). Now go look at AccountList::addNode(). The Account object you pass in, 'h', still contains that old pointer value in its 'next' field. Nothing in the code that reads in the Account object ever set its 'next' to NULL, and neither does addNode(). If you don't elect to use std::list<>, I'd recommend that you make addNode() start out by calling h->setNextAccount(NULL) before it does anything else. That takes care of the stale pointer bug.

Before I go on to the next bug, I'd like to mention that AccountList::addNode() is time complexity O(n). As your AccountList grows in size, it will take longer & longer to scan to the end of the list in order to simply add the next Account object to the list. You can do better: If you insist on adding the next Account to the end of the AccountList, then maintain a pointer in AccountList not only to the head, but also to the tail node. That way addNode() will be time complexity O(1). Alternatively, add the new Account object at the head of the list: Set the new object's 'next' to the current head, then change 'head' to be the newly added Account (this way significantly simplifies the logic for adding a new Account object to the list). Note though that looking up an account based on its account number will still be on order O(n), and if your software is handling thousands of accounts, that's some pretty bad performance for locating an account. You can get O(log n) account lookup time if you use a std::map. In that case your "addNode" time will also be O(log n) which is worse than O(1), but which will you be doing more of, adding accounts or looking up existing accounts? Probably looking up existing accounts.

OK, now the next bug: In main() when you do you check the inAccount.read() loop's exit conditions the first time, you haven't read in even the first record. No biggie there, I guess, but when you have finally read the last one, inAccount.eof() isn't yet true, so you enter the loop body again but this time inAccount.read() doesn't read anything. So acc3 hasn't changed from the last iteration. Then you still call list2->addNode(acc3) even though you didn't read anything. You add the last record twice! I'll let you figure out how to fix that problem.

Finally you have some very scary memory/object management going on. Note that you never delete your two dynamically allocated AccountLists in main(). That's not a biggie if you intend for your AccountLists to be automatically reclaimed by the system when your program exits, but if you intend to have multiple AccountLists that come & go from time to time in the life of your program, you'll need to make sure they cleanly delete their contents. It appears that you want your AccountList objects to own the Account objects assigned to them, since in your ~AccountList() destructor, you delete the 'head' Account object. Since the ~Account() destructor does not delete the 'next' object, the only Account object in the list that will get deleted is the head Account. Since AccountList only has a pointer to the head object, perhaps a valid place to delete the rest of the list would be in ~Account(), as I see you thought about doing based on the commented-out delete call. As you thought about doing, you could have each Account object delete only its own 'next', but there's a subtle bug in that approach: Each subsequent delete will add another function call to the runtime stack, recursively calling delete on each Account in the list until it gets to the end of the list. If you have thousands of Account objects, you're guaranteed to blow your runtime stack many times over. A better way to do this would be as follows:

~Account(){
    Account *victim, *itsNext = next;
    while (itsNext){
        victim = itsNext;
        itsNext = victim->next;
        victim->next = NULL; // prevent recursion & blown stack
        delete victim;
    }
    next = NULL;
}

Hmmm... I see you have an AccountList::removeAll() method which seems to be structured very similarly to my ~Account() above. Perhaps you can just call that from your ~AccountList() destructor.

Now if you decide to use std::list or std::map (I'd recommend map), you don't have to worry about memory leaks or how/when to delete, as long as your std::list or std::map is a list or map of actual Account objects, since when a list/map entry gets deleted (with erase() or when the whole list/map is deleted), the encompassed Account object(s) gets deleted right along with it. If they're a list or map of Account pointers, you'd still have to loop through and delete them manually. Unless you used a list of auto_ptrs or smart_ptrs, but I digress.

Answer 2

There are a few issues here, probably the most serious of which is the way you're trying to write the linked list to a file.

You can't write pointer members to a file and expect the same memory addresses to contain the same objects when you come to read the file back in. You should probably take a look at something like the boost serialization library or Google's Protocol Buffers.

This is partly to blame for your program hanging; when you call list2->addNode(acc3);, acc3 already has a value for next, so the while loop in addNode never returns.

Also, checking !inAccount.eof() is useless here since you don't hit eof until attempting to read for the 3rd time. The 3rd read fails (well, it reads 0 bytes), but you still call addNode. You can check how much was read by using inAccount.gcount(). If you do that immediately after reading, and if it's not the amount expected then break out of the loop, you'll avoid the extra read attempt.