My code isn't working properly as it should be. Can anyone point out the errors?

Question

I'm practicing on insertion in doubly linked list. I don't know why this code isn't working properly. The output of this code is :

position doesn't exist

position doesn't exist

1
2

As you can see it's not even printing the 3rd data. I know I am doing something wrong but I'm not able to identify my mistake in this code. Plz help me out.

#include <iostream>

using namespace std;

struct dllnode
{
    int data;
    dllnode *next,*prev;
};
class dlinked_list
{
    dllnode *head,*tail;
public:
    dlinked_list()          //Constructor
    {
        head=NULL;      
        tail=NULL;
    }
    void insertionindll(int ,int);
    static void display(dllnode *);
    dllnode* gethead()
    {
        return head;        //returning head pointer of the linkedlist
    }
    dllnode* create_node(int data);
};
dllnode* dlinked_list::create_node(int n)   //Creating a new node
{
    dllnode *temp=new dllnode;
    temp->data=n;
    temp->prev=NULL;
    temp->next=NULL;
    return temp;    //returning the address of the newly created node to line no. 39
}
void dlinked_list::insertionindll(int n, int pos)
{
    dllnode *temp;
    int k=1;
    dllnode *newnode= create_node(n);   //storing address of the newly created node
    if(!newnode)
    {
        cout<<"Memory Error";   //Checking memory error
        return;
    }
    newnode->data=n;
    if(pos==1)  //Insertion at the beginning//
    {
        newnode->next=head;     
        newnode->prev=NULL;
        if(head)        //checking if head!=NULL
            head->prev=newnode;
        head=newnode;   //now head points to the newly created node
        return;
    }
    temp=head;      //temp is now pointing to the first node
    //After this loop temp will either point to the last node 
    //or the previous node at which we want to insert newnode
    while(k<pos && temp->next!=NULL)    
    {
        k++;    //incrementing k
        temp=temp->next;    
    }
    if(k!=pos)  //checking if the position doesn't exist
    {
        cout<<"position doesn't exist"<<endl;
    }
    newnode->prev=temp;     
    newnode->next=temp->next;
    if(temp->next)
        temp->next->prev=newnode;
    temp->next=newnode;
    return;
}
void dlinked_list::display(dllnode *a)  //for displaying the created linked list
{
    dllnode *ptr;   //declaring a pointer which points to the head of the linked list
    ptr=a;
    while(ptr->next!=NULL)  //iterating untill the last node
    {
        cout<<ptr->data<<endl;  //printing data of the linked list
        ptr=ptr->next;  //pointng to the next node 
    }
    return;
}
int main()
{
    /* code */
    dlinked_list a; //creating an object a of class dlinked_list
    a.insertionindll(1,1);  //here insertionindll function is called 
    a.insertionindll(2,2);
    a.insertionindll(3,3);
    dlinked_list::display(a.gethead()); //display function is called by passing head pointer
    return 0;
}

And when you used your debugger to execute your program one line at a time, and after you used your debugger to examine the values of all variables, after each line of the program gets executed, what observations did you make? — Sam Varshavchik, Oct 04 '18 at 00:25
Suggestion: Rather than a `create_node` method, make the `dllnode` constructor smarter. Eg: `dllnode (int indata) : data(indata), prev(NULL), next(NULL) {}`. Another suggestion: `if(!newnode)` after creating a node with `new` is not necessary. `new` throws an exception if it fails, guaranteeing you'll never get here (unless you changed the behaviour of `new`, an advanced topic you shouldn't mess with for a while). — user4581301, Oct 04 '18 at 01:05
Think real long and hard on why `while(ptr->next!=NULL)` tests `ptr->next` without first making sure `ptr` isn't `NULL`. — user4581301, Oct 04 '18 at 01:09
Possible duplicate of [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) — Raedwald, Oct 04 '18 at 06:24
Thanks for your second suggestion about the memory check @user4581301 and yes there was a problem in while loop of the display function. It should have been: while(ptr!=NULL) but I'm not getting your point you made about smarter constructor. I think constructor will be called only when we create any object. In my case I'm working on a single list so how does it help making a newnode if I remove create_node method? I mean how can I call the constructor every time I need to create a new node from the main function? — Amey, Oct 05 '18 at 00:16
My first point is you don't need a separate create function in C++ in the vast majority of cases. That's the constructor's job. In idiomatic C++, no object should leave the constructor without being fully set up and ready for use or by an exception thrown because it could not be set up. If you create the object by giving the constructor all of the information it needs, the smarter constructor replaces the `create_node` function and eliminates the possibility of someone using `new dllnode ` directly and not setting up the `next` and `prev` pointers. 1/2 — user4581301, Oct 05 '18 at 01:05
2/2 With the constructor outlined in the comment above, `dllnode *newnode= create_node(n);` becomes `dllnode *newnode= new node(n);` and the `create_node` function is removed from the program. — user4581301, Oct 05 '18 at 01:06

score 0 · Accepted Answer · edited Oct 04 '18 at 06:14

0

You have not added the third node. According to your code, to the add third node, your temp should stop at second node and k will be 2.

Have this check:

 if(k!=pos-1)  //checking if the position doesn't exist

Also have a null pointer check while printing the data:

while(ptr!=null && ptr->next!=NULL)  //iterating untill the last node

edited Oct 04 '18 at 06:14

bunbun

2,595
3
34
52

answered Oct 04 '18 at 03:03

Nivedhitha N

168
1
10

My code isn't working properly as it should be. Can anyone point out the errors?

1 Answers1