Singly Linked List Infinite Loop

Question

It's been a week since i started learning about linked list and i only managed to learn about singly linked list. So today i implemented the linked list which i learned in c++ and while i tried to run it the code goes into an infinite loop of some random numbers. I tried debugging the code but i coudn't find whats so ever is wrong with the code. The code is below. Help is appreciated.Thanks

#include <iostream>
using namespace std;

struct node{
int data;
node * next;
};

class singly{
private:
node * head,*tail;
public:
singly(){
    head=NULL;
    tail=NULL;
}

void createNode(int value){
    node * temp = new node;
    temp->data=value;
    temp->next=NULL;
    if(head==NULL){
        head=temp;
        tail=temp;
        temp=NULL;
    }

    else{
        tail->next=temp;
        tail=temp;
    }
}

void display(){
    node * temp = new node;
    head=temp;

    while(temp!=NULL){
        cout << temp->data << "\t" << endl;
        temp->next=temp;
    }
}

void insert_end(int value){
    node*newnode = new node;
    node*temp = new node;
    newnode->data=value;
    newnode->next=NULL;
    temp=head;

    while(temp->next!=NULL){
        temp = temp->next;
    }
    temp->next=newnode;
}

void delete_node(){
    node*current = new node;
    node*previous = new node;
    current = head;
    while(current->next!=NULL){
        previous=current;
        current=current->next;
    }

    tail=previous;
    previous->next=NULL;
    delete current;
}
};

int main(){

singly lists;
lists.createNode(32);
lists.createNode(654);
lists.createNode(34);
lists.createNode(234);

cout<<"\n--------------------------------------------------\n";
cout<<"---------------Displaying All nodes---------------";
cout<<"\n--------------------------------------------------\n";
lists.display();
cout<<"\n--------------------------------------------------\n";
cout<<"-----------------Inserting At End-----------------";
cout<<"\n--------------------------------------------------\n";
lists.createNode(55);
lists.display();
cout<<"\n--------------------------------------------------\n";
cout<<"-----------------Deleing At End-------------------";
cout<<"\n--------------------------------------------------\n";
lists.delete_node();
lists.display();
}

At which point does it enter the infinite loop? You can't expect us to debug your entire code for you. — eshirima, Oct 23 '17 at 14:57
I recommend you do some research about what the `new` keyword does, as judging from this code you don't really have an understanding of it — xEric_xD, Oct 23 '17 at 14:58
Both `display` and `delete_node` are broken. You should only write `new` in functions that add a node to the list. You don't use `new` to create pointers – `node* temp = head;` is all you need if you want a pointer with the same value as `head`. — molbdnilo, Oct 23 '17 at 15:02
BTW, `insert_end` is needlessly complicated for a list with a tail pointer, and in addition, inserting at the end is what `createNode` does. — molbdnilo, Oct 23 '17 at 15:04

Vlad from Moscow · Accepted Answer · 2017-10-23T15:19:44.793

The member function display does not make sense. It overwtites the data member head with uninitialized newly created temp.

    node * temp = new node;
    head=temp;

so the function invokes undefined behavior.

The function can look like

void display()
{
    for ( node * temp = head; temp != nullptr; temp = temp->next )
    {
        cout << temp->data << "\t";
    }
}

Or it is better to define it the following way

std::ostream & display( std::ostream &os = std::cout )
{
    for ( node * temp = head; temp != nullptr; temp = temp->next )
    {
        os << temp->data << "\t";
    }

    return os;
}

The data member insert_end is also wrong. It does not take into account that head and tail can be equalto nullptr and does not change them.

The function can be defined the following way

void insert_end(int value)
{
    node *newnode = new node { value, nullptr };

    if ( tail == nullptr )
    {
        head = tail = newnode;
    }
    else
    {
        tail = tail->next = newnode;
    }
}

The member function delete_node firstly does not make sense for a singly-linked list and again is wrong and invokes undefined behavior. The function should remove the first node from the list.

Nevertheless if you want to remove the last node from the list then the function can look like

void delete_node()
{
    if ( head != nullptr )
    {
        tail = nullptr;
        node *current = head;

        while ( current->next )
        {
            tail = current;
            current = current->next;
        }

        if ( tail == nullptr )
        {
            head = tail;
        }
        else
        {
            tail->next = nullptr;
        }

        delete current;
    }
}

Thank bro, i'm still a noob so still in learning phase. but this helped me alot and solved my problem. — Mazhar Khan, Oct 23 '17 at 15:18

score 0 · Answer 2 · answered Oct 23 '17 at 15:14

For starters, display() is wrong. You want the update to be temp = temp->next; and it can also be initialized as node * temp = head hence not requiring the second line.

Your delete_node() can be re-written to:

if (head->next == NULL) // handles the case that it consists of 1 element
{
    delete head;
    head = NULL;
}
else
{
    node *nextToEnd = head;
    node *end = head->next;
    while (end->next != NULL)
    {
        nextToEnd = end;
        end = end->next;
    }
    delete end;
    nextToEnd->next = NULL;
}

As stated in the comments, review the use of the new keyword

Singly Linked List Infinite Loop

2 Answers2