Copying a Linked List crashes the Program

Question

I have Linked List code, the Copy_List function crashes the program and won't work: it gives no syntax error so it's logical. I'm not sure where the exact problem lies, so any help would be appreciated.

Here is the code:

#include <iostream>
using namespace std;

struct node{
   int info;
   node *link;
};


class Linked_List{
   private :

   int count;
   node *first;
   node *last;
   node *current;

   public:


   Linked_List() {

      count=0;
      first=NULL;
      last=NULL;
   }

   void Initialize_List(){

      cout<<"Enter Number OF Nodes"<<endl;
      cin>>count;

      first=last=current=new node;

      for(int i =0;i<count;i++){
         cout<<"Enter New Node Info :"<<endl;
         cin>>current->info;
         last->link=current;
         last=current;
         current=new node;
      }

      last->link=NULL;
   }

   bool Is_Empty(){
      if(first==NULL)
      {
         return true;
      }
      else{
         return false;
      }
   }

   int Front () {
      if (first != NULL)

      return first-> info; 
      else return 0;
   }

   int Back () {

      if (last != NULL)
      return last-> info;
      else return 0; 
   }

   void Insert_Last(int x){

      count++;
      current=new node;
      current->info=x;

      last->link=current;

      last=current;
      last->link=NULL;

      if(first==NULL)

      first=current;
   }

   void Delete_First(){

      if(!Is_Empty())  // Or if(first==NULL)
      { 

         node *p;
         p=first;
         first=first->link;
         delete p;
         count --;
         if(count==0)
            first=last=NULL;
      }
   }

  friend void Copy_List (Linked_List &n,Linked_List &m);

};

void Copy_List (Linked_List &n,Linked_List &m){

   Linked_List temp;
   while(!n.Is_Empty()){
      temp.Insert_Last(n.Front());
      n.Delete_First(); 
   }
   while (!temp.Is_Empty()) {

      n.Insert_Last(temp.Front());
      temp.Delete_First(); 
   }
}


void main (){
   Linked_List obj,obj2;
   cout<<"Is the list empty ?"<<"  "<<boolalpha<<obj.Is_Empty(); cout<<endl;
   obj.Initialize_List();
   cout<<"Is the list empty ?"<<"  "<<boolalpha<<obj.Is_Empty(); cout<<endl;
   Copy_List (obj,obj2);   
}

Answer 1

Suggestions for improvement:

• Add a default constructor to node so that it gets initialized properly when constructed.

  struct node{
      node(int in = 0) : info(in), link(NULL) {}
     int info;
     node *link;
  };
  

• You don't need current as a member of Linked_List. It's useful only in some functions as a function varible.

• Implement Initialize_List() using Insert_Last. That keeps the function cleaner. It also avoids redundant code.

  void Initialize_List(){
    cout<<"Enter Number OF Nodes"<<endl;
    int num;
    cin>>num;
  
    for(int i =0;i<num;i++){
       cout<<"Enter New Node Info :"<<endl;
       int info;
       cin >> info;
       this->Insert_Last(info);
    }
  }
  

• Insert_Last had assumptions about which is a valid pointer and which is not, which won't be true if you started using to from Initialize_List. It can be simplified to:

  void Insert_Last(int x){
    count++;
    node* current=new node;
    current->info=x;
  
    if ( first == NULL )
    {
       first = last = current;
    }
    else
    {
       last->link=current;
       last=current;
    }
  }
  

• Implementation of Copy_List you posted deleted all items from the first argument and put them in the second argument. I am not sure that was the purpose. If you want to keep the contents of the first argument unchanged and want to copy its contents to the second argument, an alternate method is necessary. Here's what I came up with:

  void Copy_List (Linked_List &n,Linked_List &m){
  
     Linked_List temp;
     node* current = n.first;
     for ( ; current != NULL; current = current->link )
     {
        temp.Insert_Last(current->info);
     }
  
     current = temp.first;
     for ( ; current != NULL; current = current->link )
     {
        m.Insert_Last(current->info);
     }
  }
  

• You don't have a destructor in Linked_List. The default implementation provided by the compiler won't release the memory allocated by the class. In order to deallocate the memory allocated by the class, you need to implement a destructor.

  ~Linked_List() {
    node* current=first;
    while ( current != NULL )
    {
       node* temp = current->link;
       delete current;
       current = temp;
    }
  }
  

Answer 2

In the new list last pointer is not initialized firstly:

void Insert_Last(int x) {
   ...
   last->link=current;   // for the new List last should be initialized 
   last = current;
   last->link=NULL;

Supposed change - remove the line last->link=current;

   last = current;
   last->link=NULL;