current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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

#include<iostream>

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)
    {
        cout<<"data is"<<temp->getdata();
        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);
}
share|improve this question
3  
Indenting seems very haphazard. –  Loki Astari yesterday
add comment

3 Answers

up vote 6 down vote accepted

First of all, I will start with one of the most common remarks: please, do not use using namespace std;. It is especially bad if you write it in a header file since it leads to namespace pollution and name clashes.

Instead of a method named traverse, it would be better to overload operator<< to print your list. Here is how you could adapt your function (put that code in your class):

friend std::ostream& operator<<(std::ostream& stream, const stack& sta)
{
    node *temp= sta.start;
    while(temp!=NULL)
    {
        stream<<"data is"<<temp->getdata();
        temp=temp->next;
    }
    return stream;
}

Don't forget to make it a friend function so that it can access the private members of stack.

Generally speaking, you shouldn't mark your functions inline. While it may sometimes speed up your code for small functions (your functions aren't small), most of the time, it will only make your executable larger and will not significantly speed up your code. In your case, I would remove all the inline qualifications since they won't gain you anything.

From the implementation of pop, I bet that it is meant to return the popped int since you called getdata. However, you forgot to return item. Your compiler should have warned you about the unused variable item and the non-void function pop returning nothing. If not, you should tell your compiler to give you more warnings.

You don't need to write return 0; at the end of the function main in C++. If nothing has been returned when the end of the function is reached, it automagically does return 0;. This remark only holds for the function main though. Also, you don't need to put parenthesis around the returned result; return is not a function.

share|improve this answer
    
:thanks a lot. I am a beginner, learning to write c++ code.This has helped me a lot. –  shubham yesterday
1  
Marking your functions inline will have no affect on the size or speed of your code. For code-inlining purposes the keyword inline is ignored by modern compilers (unless you explicitly force the compiler to use the hints). Humans are terrible at knowing when to inline code as a result compiler writers have long stopped trusting them and use pretty good huristics. –  Loki Astari yesterday
    
@LokiAstari Well, I was only quoting an answer on a reference question... And I actually just saw you comment on the aforementioned question. –  Morwenn yesterday
add comment
up vote 7 down vote

  • 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.

  • 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;
}

  • Instead of that node constructor, consider an initializer list:

    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.

  • To make your structure a little more intuitive, consider an empty() function:

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

    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).

share|improve this answer
    
thanks alot for your valuable comment –  shubham yesterday
add comment
up vote 6 down vote

You could improve your use of whitespace; perhaps something more like this:

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

The node class might be named Node instead (I usually use PascalCase for types and camelCase for identifiers).

node has public data member. I think it would be better if node were a private, nested type of stack (i.e. defined within and only usable by the stack class).

traverse could be const since it doesn't modify the data.

Instead of if { ... } else { ... } I usually prefer if { ... return; } ....

You don't need to set ptr->next to NULL twice (once in the constructor and again in the push function).

NULL is deprecated in C++ (prefer 0).

Your push and pop are inefficient because you push and pop from the end of the list; you can push and pop from the beginning of the list instead.

share|improve this answer
    
Thanks a lot for your comments –  shubham yesterday
    
I know nullptr is the new preferred null pointer, but why is 0 better than NULL? –  200_success♦ yesterday
    
@200_success Apparently the standard now does #define NULL 0 ... so NULL is the same as 0 ... but NULL is a (unnecessary/useless) C-style macro ... see e.g. stroustrup.com/bs_faq2.html#null ... Perhaps NULL used to cause problems years ago when a "C/C++" compiler defined it as #define NULL (void*)0 and so NULL couldn't be assigned to e.g. a variable of type char*. –  ChrisW yesterday
    
+1 for the suggestion to push/pop at the start of the list rather than the end. For large stacks this is a major speed improvement. –  Corey yesterday
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.