-2

So I am trying to figure out on my own where my linked list program is going wrong. The head is somehow getting updated. I know its a tiny mistake but I just not finding where I am going wrong. is it related to global declaration of the variable ? 

#include <iostream>
using namespace std;
struct node {
    int data;
    struct node* next;
}* head = NULL;

void insert()
{
    struct node *newnode, *temp;
    temp = (struct node*)malloc(sizeof(struct node));
    newnode = (struct node*)malloc(sizeof(struct node));
    cout << "Enter the element in the Linked list" << endl;
    cin >> newnode->data;
    newnode->next = NULL;
    if (head == NULL) {
        head = newnode;
        temp = head;
    }
    else {
        temp->next = newnode;
        temp = newnode;
    }
}
void display(struct node* p)
{
    while (p != NULL) {
        cout << " " << p->data << endl;
        p = p->next;
    }
}
int main()
{
    int ch;
    do {
        cout << "1.To Enter element in the Linked List" << endl;
        cout << "2.To DIsplay Element in the Linked List" << endl;
        cout << "3.To exit" << endl;
        cin >> ch;
        switch (ch) {
        case 1: {
            insert();
            break;
        }
        case 2: {
            display(head);
            break;
        }
        }
    } while (ch != 3);

    return 0;
}
Alan Birtles
  • 32,622
  • 4
  • 31
  • 60
Glorious15
  • 3
  • 3
  • 3
    a) this is not C ! dont tag unrelated languages please b) What is wrong with the code? – 463035818_is_not_an_ai Jan 09 '20 at 14:10
  • Your insert can not work on the second or subsequent inserts. You need to actually traverse the list from head to add a new tail node or keep a pointer to the previous tail. You also allocate 2 nodes for every insert. – drescherjm Jan 09 '20 at 14:13
  • `temp = head;` in insert is pointless and leaks memory. It is pointless because when insert is done `temp` no longer exists. It leaks memory because `temp` pointed to an allocated node that is no longer accessible after the ``temp = head;` statement. However you do leak memory regardless. – drescherjm Jan 09 '20 at 14:16
  • 1
    My advice when it comes to pointers is to work it out with pen and paper before writing code. – drescherjm Jan 09 '20 at 14:21
  • 4
    You shouldn't use malloc and free. Use new and delete or use smart pointers/STL containers. This code is C with iostream. You should either replace iostream by stdio.h and make it a real C program or you should rewrite the program to modern C++. – Thomas Sablik Jan 09 '20 at 14:23
  • Read [*How to debug small programs*](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). Enable all warnings and debug info (so with [GCC](http://gcc.gnu.org/) compile with `g++ -Wall -g` ) – Basile Starynkevitch Jan 09 '20 at 14:30
  • About [using namespace std](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice)... – Aconcagua Jan 09 '20 at 14:40
  • You should prefer C++ *keywords* (`nullptr`) over old (obsolete?) C *macros* (`NULL`). Apart from, you can (but don't have to) check for null pointers more conveniently: `while(p)`, `if(!head)`. – Aconcagua Jan 09 '20 at 14:42

1 Answers1

2

There are a few issues here, but the biggest one is you aren't appending to the end of your linked list. You need to find where the ->next node is NULL for an item in your list, indicating the final item. Then your new node should become ->next for that item. Also, be careful to check for head being NULL, in which case your new node should become head. You can modify the insert() function to work appropriately as so. 

void insert()
{
    struct node *newnode, *temp;
    newnode = (struct node*)malloc(sizeof(struct node));
    cout << "Enter the element in the Linked list" << endl;
    cin >> newnode->data;
    newnode->next = NULL;
    if (head == NULL) {
        head = newnode;
    } else {
        temp = head;
        while (temp->next != NULL) {
            temp = temp->next;
        }
        temp->next = newnode;
    }
}

Note: You are mixing elements of C and C++ (malloc is a C concept for dynamic memory allocation while new is a C++ concept), and as someone pointed out in the comments you should likely stick to one (unless you're taking a programming course and your professor wanted you to use certain methods).

h0r53
  • 3,034
  • 2
  • 16
  • 25