Back Traversal issue in linked list

Question

following is C++ written linked list with Node class and main function. List is traversing forward using "next()" function but producing execution time error when traverse back using "back()".

#include <iostream>
using namespace std;

class Node {
    public:
        int object;
        Node *nextNode;
        Node *prevNode;
        
    public:
        
        int get(){
            return object;
        }
        
        void set(int object){
            this->object = object;
        }
        
        Node* getNext(){
            return nextNode;
        }
        
        void setNext(Node *nextNode){
            this->nextNode = nextNode;
        }
        
        Node* getPrev(){
            return prevNode;
        }
        
        void setPrev(Node *prevNode){
            this->prevNode = prevNode;
        }
        
    
};


class List {
    public:
        Node* headNode;
        Node* currentNode;
        int size;
    
    public:
        
        List(){
            headNode = new Node();
            headNode->setNext(NULL);
            headNode->setPrev(NULL);
            currentNode = NULL;
            int size = 0;   
        }
        
        void add(int addObject){
            Node* newNode = new Node();
            newNode->set(addObject);
            
            if(currentNode != NULL){
                newNode->setNext(currentNode->getNext());
                newNode->setPrev(currentNode);
                currentNode->setNext(newNode);
                currentNode = newNode;
                            
            }
            else {
                newNode->setNext(NULL);
                newNode->setPrev(headNode);
                headNode->setNext(newNode);
                currentNode = newNode;
            }
            
            size++;
    
        }
        
        int get(){
            if(currentNode != NULL) {
                return currentNode->get();
            }
        } 
        
        bool next(){
            if(currentNode == NULL) return false;
            
            currentNode = currentNode->getNext();
            
            if(currentNode == NULL) return false;
            else                    return true;
        
        }
        
        bool back(){
            if(currentNode == NULL) return false;
            currentNode = currentNode->getPrev();
            
            if(currentNode == NULL) return false;
            else return true;
        }
        
        void start(){
            currentNode = headNode;
        }
        
        void remove() {
            if (currentNode != NULL && currentNode != headNode){
                delete currentNode;
                size--;
            }
        }
        
        int length() {
            return size;
        }
        
};


int main(){
    
    List list;
    
    list.add(5);
    list.add(13); 
    list.add(4);
    list.add(8);
    list.add(48);
    list.add(12); 
    
    list.start(); 
    
    while(list.next()){
        cout<<endl<<"Element: " << list.get() << endl;
    }
     
    cout<<endl<<"BACK"<<endl;

    while(list.back()){
        cout<<endl<<"Element: " << list.get() << endl;
    } 
}

Back() function should traverse the list in opposite direction (from end to start).. reverse manner. Sometime this code hangs the CPU and sometimes it only runs next() function, and for back() function it remains silent without doing anything.

maybe its related with the fact that your get function doesn't return anything if Null occurs — Spyros Mourelatos, Sep 18 '20 at 11:03
Did you try to debug your code and check that the pointers point to expected locations? — Quimby, Sep 18 '20 at 11:03
When you insert new nodes in `add`, you update the `current->next` pointer. But you fail to update `current->next->prev`, so the reverse structure is never correctly formed. — Frodyne, Sep 18 '20 at 11:05
@Asad Razaq Just throw that bad code into the trash can and rewrite the list implementation anew.:) — Vlad from Moscow, Sep 18 '20 at 11:06
Did not find what? Step through the code and see if it does what you expect it to. — Quimby, Sep 18 '20 at 11:06
@Asad Razaq There is no great sense to use the pointer current. The list should have two pointers: to the head node and to the tail node. — Vlad from Moscow, Sep 18 '20 at 11:08
@Frodyne I did consider the prev using this line: newNode->setPrev(currentNode); — Asad Razaq, Sep 18 '20 at 11:08
@Asad Razaq This code snippet in the constructor headNode = new Node(); //... currentNode = NULL; is logically inconsistent and only confuses readers of the code. — Vlad from Moscow, Sep 18 '20 at 11:10
I'm not sure why it can hang, but `back()` will always will return `false` in this code because `currentNode` will be `NULL` (BTW should use `nullptr` instead) after last call to `next()` in first cycle. Also you have bug in `remove()` function: you need to adjust pointers around deleted element. — sklott, Sep 18 '20 at 11:11

Martin Morterol · Accepted Answer · 2020-09-19T08:54:30.377

First, let's fix the code:

 bool next(){
        if(currentNode == nullptr)  return false; 
        // if next is null we are at the end, don't go futher
        if (  currentNode->getNext() == nullptr ) return false;
        currentNode = currentNode->getNext();
        return true;
    }
    
    bool back(){
        if(currentNode == nullptr) return false;
        
        // if prev is head, we are at the start, stop here 
        if ( currentNode->getPrev() == headNode) return false;
        currentNode = currentNode->getPrev();
       
        return true;
    }

The logic:

// we are at the last element, so we have to print BEFORE going back
do{
    cout<<endl<<"Element: " << list.get() << endl;
} while (list.back());

Demo live

Warnings:

warning: unused variable 'size'

In this case, it's ok to have this warning, you may use the method length() if you want to get rid of it.

In member function 'int List::get()': warning: control reaches end of non-void function [-Wreturn-type]

In your get method, if(currentNode == nullptr ) you return nothing and it will cause a bug. One way to fix this is to throw an exception.

int get(){
    if(currentNode == nullptr ) {
       throw std::logic_error("CurrentNode is null");
    }
    return currentNode->get();
}

Personally I think the best solution is: code all member functions of List so that currentNode cannot be null.

Memory Management

You create your node with new but you never use delete so you have a memory leak. Check out valgrind (site web or this nice post), it's super helpful.

valgrind ./leaky --leak-check=full
....
==3225== HEAP SUMMARY:
==3225==     in use at exit: 168 bytes in 7 blocks
==3225==   total heap usage: 9 allocs, 2 frees, 73,896 bytes allocated
==3225== 
==3225== LEAK SUMMARY:
==3225==    definitely lost: 24 bytes in 1 blocks 
==3225==    indirectly lost: 144 bytes in 6 blocks
==3225==      possibly lost: 0 bytes in 0 blocks
==3225==    still reachable: 0 bytes in 0 blocks
==3225==         suppressed: 0 bytes in 0 blocks

So yes, valgrind found a leak.

You need to add a destructor.

~List(){
       // first be sure that we are at one end
       while (next()) {}
        
       while (back())
       {
           std::cout << "delete the node we just left : " << currentNode->getNext()->get() << std::endl;
           delete currentNode->getNext();
       }
       // don't forget this one (without valgrind I will have miss it!)
       delete currentNode;

       std::cout << "finaly we clear the head" << std::endl;
       delete headNode;
}

But now if we write :

List list2 = list;

We got :

double free or corruption (fasttop)

Because we have 2 objects trying to delete the same memory.

We can just forbid the copy :

 List(const List&) = delete;
 List& operator=(const List&) = delete;

Usually most memory management is done via smart pointer.

Visibility:

Use private for your attributes :

private :
    int object;
    Node *nextNode;
    Node *prevNode;

and

private:
    Node* headNode;
    Node* currentNode;
    size_t size = 0;

The final version: Demo

Check with valgrind:

==3532== HEAP SUMMARY:
==3532==     in use at exit: 0 bytes in 0 blocks
==3532==   total heap usage: 9 allocs, 9 frees, 73,896 bytes allocated
==3532== 
==3532== All heap blocks were freed -- no leaks are possible

No leak, all good ;)

Hope it helps !

Back Traversal issue in linked list

1 Answers1