Singly linked list following a weird display pattern

Question

Following code works perfectly if I make 1 node and display. However, if I insert 2 or more nodes then only the last node entered is displayed and the ones that were already in linked list. E.g if I have linked List of 3 1 2 4 and I enter 2 1 3 consecutively and call the display function then output = 3 1 2 4 3.

    struct node
{
    char info;
    node* link;
}*f,*nn,*p,*c;//variables to make new node, head and control previous and current

void create(char inf)
{

    if(f=='\0')
    {
        nn=new node[sizeof(struct node)];
        nn->info=inf;
        nn->link='\0';
        f=c=p=nn;
    }
    else
    {

        nn=new node[sizeof(struct node)];
        nn->info=inf;
        p->link=nn;
        c=nn;
        c->link='\0';
    }
}

void display()
{

    c=p=f;
    while(c!='\0')
    {
        cout<<c->info<<" ";
        p=c;
        c=c->link;
    }
    cout<<endl;

}

int main()
{
    while(3)
    {
        int sw=0;
        cout<<"Enter \n1. to create list \n2. to display list"<<endl;
        cin>>sw;
        switch(sw)
        {
            case 1:{
            char info;
            cout<<"Enter info!"<<endl;
            cin>>info;
            create(info);
            break;
            }
            case 2:display();
            break;
            default:
            cout<<"Wrong entry, try again!"<<endl;
        }
    }


}

Pardon if something is so obvious since i have tried my best to find the solution.

Answer 1

Problems that I see:

1. f, nn, p, and c are not initialized.

   Change the lines to:

   struct node
   {
       char info;
       node* link;
   };
   
   node *f = NULL;
   node* nn = NULL;
   node* p = NULL;
   node* c = NULL;
   

2. Change the line

   if(f=='\0')
   

   to

   if (f == NULL)
   

   This is a style change but more readable. Use '\0' only for comparing characters. Use NULL for comparing pointers.

3. The following line does not seem right.

   nn=new node[sizeof(struct node)];
   

   It allocates an array of object containing sizeof(struct node) items and returns a pointer to that array. You just need one object. Change the line to:

   nn=new node;
   

   There are two such lines, one in each of the blocks under if (f=='\0').

4. You are not making the links correctly under the else block.

   nn=new node[sizeof(struct node)];
   nn->info=inf;
   p->link=nn;  // This is good only for the second item.
                // When you add the third item, the second item becomes an orphan.
                // When you add the fourth item, the third item becomes an orphan.
   
   c=nn;
   c->link='\0';
   

   What you need is:

   nn=new node;
   nn->info=inf;
   nn->link = NULL;
   c->next = nn;
   c = nn;
   

5. You are modifying the global variables p and c in display. If you try to add any more items after the call to display, you will get unexpected behavior. Using local variables in display will prevent that problem.

   void display()
   {
       Node* n = f;
       while( n != NULL)
       {
           cout << n->info << " ";
           n = n->link;
       }
       cout << endl;
   }
   

Suggested cleanup

You don't need the variable nn in the global scope. It's used only in create. Remove it from the global scope and put it in create.

You don't need the global variable p at all. The only place you thought it would be useful is in the else block. But, as you can see, it is not needed there.

Use NULL instead of '\0' to compare pointers.

Answer 2

The problem is in this line:

      p->link=nn;

You are adding new node in same position and not updating 'p'. So if you add 2 nodes, the second one is added where the first one was, and the first one is lost. Hence you always see only the last one added.