Representing a stack using pointers

Question

I can't seem to understand pointers in C++, I hope my example could help you guide me I have the following code:

   #include "stdafx.h"
   #include <iostream>

   using namespace std;

first class:

    class Node
    {
    private:
        int value;
        Node *next;
    public:
        Node()
       {
            value = 0;
       }
       ~Node()
       {

       }
       void setValue(int x)
       {
           value = x;
        }
        int getValue()
        {
           return value;
        }
        void setNext(Node x)
        {
            next = &x;
        }
        Node getNext()
        {
            return *next;
        }
     };

Second class:

    class Stack
    {
    private:
       Node *top;
    public:
        Stack()
        {
           top = NULL;
        }
        ~Stack()
        {

        }
        Node pop()
        {
            Node *temp = top;
            top = &top->getNext();
            return *temp;
        }
        void push(Node &x)
        {
            x.setNext(getTop());
            setTop(x);
        }
        Node getTop()
        {
            return *top;
        }
        void setTop(Node x)
        {
            top = &x;
        }
    };

and the main function:

    int main()
    {
       Node a;
       Node b;

       a.setValue(1);
       b.setValue(2);

       Stack s;

       s.push(a);
       s.push(b);

       cout << s.pop().getValue() << endl;
       cout << s.pop().getValue() << endl;
       return 0;
   }

I keep getting random numbers, I know I have a problem using objects and pointers, I just can't figure out what my problem is

Answer 1

Assigning a pointer to the arguments to member variables is completely useless because the argument will vanish on returning from the function and the pointer will become meaningless.

class Node
{
private:
    int value;
    Node *next;
public:
    Node()
   {
        value = 0;
   }
   ~Node()
   {

   }
   void setValue(int x)
   {
       value = x;
    }
    int getValue()
    {
       return value;
    }
    void setNext(Node* x)      // setNext takes a pointer, not a Node by value.
    {
        next = x;
    }

    Node *getNext()
    {
        return next;
    }
};

and

class Stack
{
private:
   Node *top;
public:
    Stack()
    {
       top = NULL;
    }
    ~Stack()
    {

    }
    Node pop()
    {
        Node *temp = top;
        top = top->getNext();
        return *temp;
    }
    void push(Node &x)
    {
        x.setNext(getTop());
        setTop(&x);
    }
    Node *getTop()
    {
        return top;
    }
    void setTop(Node *x)    // setTop takes a pointer, not a Node by value.
    {
        top = x;
    }
};

I'm afraid this implementation may be dangerous for application, but this will work here.

Answer 2

Make your getters/setters return pointers instead of objects. This code:

        Node *temp = top;
        top = &top->getNext();
        return *temp;

Will assign temp to top, then assign top to an address of a temporary object created by getNext() and then return the object temp is pointing to. Using top a next time will result in undefined behaviour because top is now a dangling pointer.

Node:

class Node
{
private:
    int value;
    Node *next;
public:
    Node()
   {
        value = 0;
   }
   ~Node()
   {

   }
   void setValue(int x)
   {
       value = x;
    }
    int getValue()
    {
       return value;
    }
    void setNext(Node* x)
    {
        next = x;
    }
    Node* getNext()
    {
        return next;
    }
 };

Stack:

class Stack
{
private:
   Node *top;
public:
    Stack()
    {
       top = NULL;
    }
    ~Stack()
    {

    }
    Node pop()
    {
        Node *temp = top;
        top = top->getNext();
        return *temp;
    }
    void push(Node* x)
    {
        x->setNext(getTop());
        setTop(x);
    }
    Node* getTop()
    {
        return top;
    }
    void setTop(Node* x)
    {
        top = x;
    }
};

Answer 3

Node::setNext, Node::getNext, Stack::getTop and Stack::setTop all return/pass by values, which go out of scope at some point. There should be references, which will result in your stack pointing to original Node a and Node b, which go out of scope at the end of main when they're no longer used.

But this is all wrong. You should be storing copies on the heap, not just pointer to objects with automatic storage duration.