Implementation of stack using pointers

Please review my code and let me know how I can possibly improve it.

#include

using namespace std;

class node
{
    private:
                    int data;
    public:
                  node *next;
                    node(int element)
                    {
                            data=element;
                            next=NULL;
                    }
                    int getdata()
                    {
                        return(data);
                    }                               
};

class stack
{
    private:
                    node *start;
    public:
                    void push(int element);
                    int pop();
                    void traverse();
                    stack()
                    {
                        start=NULL;
                    }
};

inline void stack::push(int element)
{
    node *ptr;
    node *temp=start;
    ptr=new node(element);
    if(start==NULL)
    {
        start=ptr;
        ptr->next=NULL;

    }
    else
    {
            while(temp->next!=NULL)
            {
                temp=temp->next;    
            }
            temp->next=ptr;
            ptr->next=NULL; 
    }
}

inline int stack::pop()
{
    node *temp=start;
    node *top=start;
    while(temp->next!=NULL)
    {
        temp=temp->next;
    }
    while(top->next!=temp)
    {
        top=top->next;
    }
    top->next=NULL;
    int item=temp->getdata();
    delete (temp);
}

inline void stack::traverse()
{
    node *temp=start;
    while(temp!=NULL)
    {
        coutgetdata();
        temp=temp->next;
    }
}

int main()
{  
  stack a;
    for(int i=0;i<10;i++)
    {
        a.push(i);
    }
    a.traverse();
    for(int i=0;i<5;i++)
    {
        a.pop();
    }
    a.traverse();
    return(0);
}

-
Please fix your indentation all over the header. It should be consistent with everything else.

-
You don't need the return 0 at the end of main(). Reaching this point implies successful termination, and the compiler will just insert it for you.

-
In common stack implementations, pop() is void (merely pops off the top element). This is useful as it'll be much easier to terminate the function early if the stack is empty.

For getting an element from the top, you would also have a top() function. It will also have to account for an empty stack, but it would be best to check for an empty list first.

-
As @ChrisW has mentioned, the node class should be defined within the stack class as private. Also, there shouldn't be anything public inside the node class. If you put it within stack, its private data can still be accessed without an accessor.

-
A member function like getdata() should be const as it doesn't modify any data members. You also don't need the () around data.

int getdata() const
{
    return data;
}

Moreover, you shouldn't have this function at all. A basic node structure should just have a data field and a pointer to the next/previous node in the list. As mentioned, anything containing this class will have access to its data.

-
Prefer an initializer list instead of the constructor assignments:

node(int element) : data(element), next(NULL) {}

One advantage to this is that if you have any const data members, you'll be able to initialize them within this list. But with a constructor like yours, that won't work.

-
You must have a destructor for a data structure that utilizes manual memory allocation. What happens if your structure object goes out of scope, but you haven't already popped every element? You'll have a memory leak.

To prevent this, have the destructor traverse the list, using delete on each node.

-
You can make your structure more intuitive and safer with a empty() function:

bool empty() const { start == NULL; }

This can just be defined within the class declaration and should be public.

It is assumed that the list is empty if the head points to NULL, otherwise you can instead compare a size member to 0 (if you even want to bother with maintaining a size).

StackExchange Code Review Q#46048, answer score: 15