0

Being a beginner I tried this single linked list program to accept and display first to last elements.I can't figure out what is wrong.After you run it the program stops responding after taking in the first element. I am not very familiar with the language and am new to pointer concept. This was an assignment work.

#include <iostream>
using namespace std;

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

class alpha
{
  public:
    node* head;
    node* last;
    node* n;
    node* p;
    int x;
    char ch;

    void input()
    {
        cout << "Enter the element..";
        cin >> x;
        insert(x);
        cout << "Do you want to add more?";
        cin >> ch;

        if (ch == 'y')
        {
            input();
        }
        else
        {
            display();
        }
    }
    void insert(int x1)
    {
        n = new node;
        n->data = x1;

        if (head == NULL)
        {
            head = n;
            last = n;
        }
        else
        {
            n->next = NULL;
            last->next = n;
            last = n;
        }
    }
    void display()
    {
        p = head;

        while (p != NULL)
        {
            cout << p->data;
            p = p->next;
        }
    }
};

int main()
{
    alpha o;
    o.input();
    return 0;
}
HolyBlackCat
  • 78,603
  • 9
  • 131
  • 207
Pushkar Mahajan
  • 121
  • 6
  • You should format your code properly when asking for help here. This is hardly readable. – HolyBlackCat Feb 08 '18 at 11:34
  • 2
    Reading [a couple of good beginners books](https://stackoverflow.com/a/388282/440558) and [learning how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) would be a good start. Hint: Uninitialized member variables *stay* uninitialized unless you explicitly initialize them. – Some programmer dude Feb 08 '18 at 11:35
  • 2
    Figure out how to use a debugger. It catches some runtime problems and you can press pause when the program gets stuck to check where it got stuck and go step by step to see why it can't progress. – nwp Feb 08 '18 at 11:35
  • 2
    You have an uninitialized pointer. Dereferencing uninitialized pointer causes undefined behavior. First attempt at doing so is in this statement: `last->next = n;` – Ron Feb 08 '18 at 11:37
  • Say, when you insert your first node (`head == NULL`) what is the value of n->next ? what might happen in `display()` when `p` ends up being `n->next` ? – Caninonos Feb 08 '18 at 11:40
  • Okay i will take into consideration all this while posting thank you for your help. – Pushkar Mahajan Feb 08 '18 at 11:42
  • Btw, don't forget about releasing the memory you allocate with new. – Caninonos Feb 08 '18 at 11:42

1 Answers1

0

the big mistake already pointed out is the absence of an initialization by a constructor. Also I suggest to move some data member to private, and make some of them local.

#include <iostream>
using namespace std;

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

class alpha
{
private:
    node* head;
    node* last;

public:
    alpha() {
        head = NULL;
        last = NULL;
    }

    void input()
    {
        int x;
        char ch;

        cout << "Enter the element..";
        cin >> x;
        insert(x);
        cout << "Do you want to add more?";
        cin >> ch;

        if (ch == 'y')
        {
            input();
        }
        else
        {
            display();
        }
    }
    void insert(int x1)
    {
        node* n = new node;
        n->data = x1;

        if (head == NULL)
        {
            head = n;
            last = n;
        }
        else
        {
            n->next = NULL;
            last->next = n;
            last = n;
        }
    }
    void display()
    {
        node* p = head;

        while (p != NULL)
        {
            cout << p->data;
            p = p->next;
        }
    }
};

int main()
{
    alpha o;
    o.input();
    return 0;
}

As someone suggested, please implement a destructor ~alpha(), in order to avoid leaks of node instances.

Stefano Buora
  • 1,052
  • 6
  • 12
  • 1
    @PushkarMahajan If you implement a destructor you will have to implement the other 4 too, see [rule of 3/5/0](http://en.cppreference.com/w/cpp/language/rule_of_three). That last one, rule of 0 would actually be preferable. Replace `node*` with `std::unique_ptr` and don't write a destructor. – nwp Feb 08 '18 at 11:52
  • @nwp, replacing `node*` with a unique_ptr it's not immediate. Think about the `node* last` and the node pointed by `node *next` of the second-last element, they are pointing to the same resource. BTW, good point about 3/5/0. – Stefano Buora Feb 08 '18 at 13:18