C++ linkedList head returns NULL even though list is not empty

Question

I am trying to implement the pop function of a linked list in C++. My Node and Linked List classes look like this:

//Node class 
template <class T> 
class Node{
    public:
        T data; 
        Node<T> *next;

        Node(T data, Node<T> *next);
        ~Node();
};

template <class T>
Node<T>::Node(T data, Node<T> *next){
    this->next = next;
    this->data = data;
}

template <class T>
Node<T>::~Node(){
    delete this;
}

//LinkedList class
template <class T> 
class  LinkedList{ 
    public:
        //fields
        Node<T> *head;
        int size;

        //methods
        LinkedList();
        void push(T data);
        T pop();   
        void printList();

};

template <class T>
LinkedList<T>::LinkedList(){
    this->head = NULL;
    this->size = 0;
}

template <class T>
void LinkedList<T>::printList(){
    int i = 1;
    while(head){
        std::cout<<i<<": "<<head->data<<std::endl;
        head = head->next;
        i++;
    }
}

int main(){ 
    LinkedList<int> list;

    for (int i = 1; i < 6; ++i)
        list.push(i);

    list.printList();

    for (int j = 0; j < 3; ++j){
        int output=list.pop();
        printf("popped: %d\n", output);
    }

    list.printList();
    return 0;
}

Below is my pop function. The problem is that this->head is returning NULL. hence I cannot change its value or access its data field. I used print statements to find out that this->head is returning NULL. How can I resolve this issue?

template <class T>
T LinkedList<T>::pop(){
    Node<T> *h = this->head;

    //if list is empty
    if (this->size==0){
        return 0;
    }

    //if list has only one node
    if (this->size==1){
        T ret = h->data; 
        this->head = NULL;
        this->size --; 
        return ret; 
    }

    //if list has multiple nodes
    else{
        T ret = this->head->data;
        this -> head = h->next;
        return ret;
    }

    h.~Node<T>();

}

Below if my push function. I have tested this function and it works fine, but please let me know if it is not handling node pointers properly.

template <class T>
void LinkedList<T>::push(T data){
    Node<T> *n = new Node<T>(data, this->head);
    this->head = n;
    this->size++;   
}

*h.~Node();* compiles for you? Anyway, why do you call destructor explicitly like that, what is the purpose? — hyde, Nov 16 '17 at 22:35
I defined a function for ~Node() which I did not include. I was trying to stop a memory leak by freeing the node pointer *h, but now that I look at my code, I am not sure if I need it at all. Thanks for pointing it out! — robt, Nov 16 '17 at 22:40
*I was trying to stop a memory leak* -- You stop the memory leak by properly handling the resources in your class, not by explicitly calling the destructor. — PaulMcKenzie, Nov 16 '17 at 22:56
Recommend a read through of [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) to reduce future problems. — user4581301, Nov 16 '17 at 22:59
`push` looks good assuming `Node(T data, Node *next)` is good and `LinkedList();` sets `head` to `NULL`. — user4581301, Nov 16 '17 at 23:03

Remy Lebeau · Accepted Answer · 2017-11-17T00:35:54.633

template <class T>
Node<T>::~Node(){
    delete this;
}

This is so very very wrong. You need to get rid of it completely.

More importantly, printList() is modifying head when it shouldn't be. That is why head is NULL when pop() is called. Use a local Node* variable to iterate the list:

template <class T>
void LinkedList<T>::printList(){
    int i = 1;
    Node<T> *n = head; // <-- here
    while(n){
        std::cout << i << ": " << n->data << std::endl;
        n = n->next;
        i++;
    }
}

Also, pop() is not freeing nodes correctly (if at all), and not always decrementing size. It should look more like this instead:

template <class T>
T LinkedList<T>::pop(){
    Node<T> *h = this->head;

    //if list is empty
    if (this->size==0){
        return T(); // <-- not 0! or throw an exception instead...
    }

    //if list has only one node
    if (this->size==1){
        T ret = h->data; 
        this->head = NULL;
        this->size--; 
        delete h; // <-- add this!
        return ret; 
    }

    //if list has multiple nodes

    T ret = this->head->data;
    this->head = h->next;
    this->size--; // <-- add this!

    delete h; // <-- not h.~Node<T>()!

    return ret; // <-- moved down here!
}

Or simpler, this (no need to handle the size==1 case separately):

template <class T>
T LinkedList<T>::pop(){
    Node<T> *h = this->head;

    //if list is empty
    if (!h){
        return T();
    }

    //if list has any nodes

    T ret = h->data;
    this->head = h->next;
    this->size--;
    delete h;

    return ret;
}

C++ linkedList head returns NULL even though list is not empty

1 Answers1