CODEWITHSUNDEEP
c++memory-managementlinked-list
SvenG
SvenG

Reputation: 65

How to correctly delete nodes in singly-linked-list c++

I'm having difficulty with deleting my list through a destructor. I have a class List(which acts as header for the list) and class Element(which is actually a node). How can I delete Element chain in the List destructor?

This is custom implementation in c++ for a job interview. I'm trying to use delete_list() function to delete the entire list after the list lifetime expires. I'm not sure how to approach this problem entirely. I also have an overloaded operator<< to print out the list and after the first print the list is completely gone.

   /****************************************************************
   * Template Element class, these make up the chain for the list
   *****************************************************************/
   template<class T>
   class Element{
   public:
       T element;
       Element* next;

       Element() { element = 0; next = NULL; }
       Element(const Element<T>& other): element(other.element), next(other.next) {};
          ~Element() {  next = nullptr; }
   };

   /****************************************************************
   * Template List class
   *****************************************************************/
   template<class N>
   class List{
       Element<N>* first;
       unsigned size;

   public:

   /****************************************************************
   * Constructors and Destructors
   *****************************************************************/
   List() { size = 0; first = nullptr; };
   /* Constructor with input - for memory preallocation */
   List(Element<N>* mem_destination){ size = 0; first = mem_destination; };
   List(const List<N>& other): first(other.first), size(other.size) {};
   ~List(){ delete_list(); }
    void delete_list()
   {

       Element<N>* iter;
       size = 0;

       while(first != nullptr)
       {

           iter = first->next;
           delete first;
           first = iter;

           }
       if(iter != nullptr)
       {
           delete iter;
           iter = nullptr;
       }
       if(first != nullptr)
           {
           delete first;
           first = nullptr;
       }


   }
   friend std::ostream& operator<< (std::ostream& os, const List lista){
       Element<N>* iter = lista.first;
       os << "size: " << lista.size << std::endl;
       while(iter != NULL){
           os << iter->element << std::endl;
           iter = iter->next;
       }
       if(iter != nullptr)
           iter = nullptr;
       return os;
   }
...

Upvotes: 1

Views: 264

Answers (1)

john
john

Reputation: 87959

Too much code, this works

void delete_list()
{
    size = 0;
    while (first != nullptr)
    {
        Element<N>* next = first->next;
        delete first;
        first = next;
    }
}

Your version was OK for the first loop, but then for some reason you decided you had to delete iter even though it is only a working variable, not part of the list, and then you decided to delete first again. I don't know why you felt the need to do that.

Incidentally this is a serious bug

List(const List<N>& other): first(other.first), size(other.size) {}

When you copy a list you need to allocate a new set of nodes, otherwise you end up with two lists sharing the same set of nodes and no way of telling when a node is safe to delete. You probably need to read up on the rule of three.

Upvotes: 3

Related Questions