0

Can anyone point out what might be the issue with this recursion in print list? I get the last element being printed indefinitely.

class Node
{
    public:
    Node* next;
    int data ;
    
     Node()
    {
        this->data = 0;
        this->next = NULL ;
    }
Node add(int data)
{
    Node* node_t;
    node_t = new Node();
    node_t->data = data;
    node_t->next = NULL;
    this->next = node_t ;
            
}
    void print_list()
    {
        cout<<"data in list is "<< this->data << endl ;
        while(this->next != NULL)
            this->next->print_list();
    }
3
  • 1
    You should either use loops, or you should use recursion. You should not use both. Commented Sep 25, 2021 at 18:06
  • 1
    I also suggest you split responsibility into separate node and list classes. Commented Sep 25, 2021 at 18:07
  • while(this->next != NULL) should be an if (). Think about what happens in the loop minus the recursion part. When and where do you alter this->next to allow the loop to adance? Commented Sep 25, 2021 at 18:08

1 Answer 1

3

So in member function print_list() you use a while loop.

void print_list()
{
    cout<<"data in list is "<< this->data << endl ;
    while(this->next != NULL)
        this->next->print_list();
}

But the think is because you use a loop here, each node would call the function indefinitely, because the next variable if NULL, will never be assigned anything else, so you'll keep getting the second element printed.

So your function should look like that:

void print_list() {
        cout<<"data in list is "<< this->data << endl;
        if (this->next != NULL)
                this->next->print_list();
}

Also this:

void add(int data)
{
    Node* node_t;
    node_t = new Node();
    node_t->data = data;
    node_t->next = NULL;
    this->next = node_t ;
            
}

Should be that:

void add(int data)
{   
    Node* node_t;
    Node* last = this;
    while(last->next)
            last = last->next;
    node_t = new Node();
    node_t->data = data;
    node_t->next = NULL;
    last->next = node_t ;
}

Or that, if you really like recursion:

void add(int data)
{
    Node* last = this;
    if (next)
            return next->add(data);
    Node* node_t;
    node_t = new Node();
    node_t->data = data;
    node_t->next = NULL;
    last->next = node_t ;
}

Because if you do not append to the last element, you'd end up rewriting the second element, which also causes a memory leak.

I also hope you have a destructor to free all the memory you've allocated.

Edit: I noticed, that you have a non void return type for methods that don't have a return statement. Make sure you either return something or return type is void.

Sign up to request clarification or add additional context in comments.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.