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.

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
5  
Indenting seems very haphazard. –  Loki Astari Apr 2 at 20:17

3 Answers 3

up vote 8 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 Apr 2 at 14:27
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 Apr 2 at 20:19
    
@LokiAstari Well, I was only quoting an answer on a reference question... And I actually just saw you comment on the aforementioned question. –  Morwenn Apr 2 at 22:49
  • 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).

share|improve this answer
    
thanks alot for your valuable comment –  shubham Apr 2 at 14:33

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 Apr 2 at 15:11
    
I know nullptr is the new preferred null pointer, but why is 0 better than NULL? –  200_success Apr 2 at 21:34
    
@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 Apr 2 at 21:57
    
+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 Apr 3 at 3:50
    
I have to disagree with 0 over NULL. NULL conveys meaning that 0 doesn't. Though the same as 0, NULL still mentally screams "hey this is a pointer!" I tend to agree that macros are evil, but I think NULL is an exception. T* ptr = 0; just doesn't have the same mental effect on me that T* ptr = NULL; does. Not to mention that compilers are still allowed to generate notices about NULL. A compiler can still have internal special meaning for NULL and treat it like it's a pointer type just in warning form instead of an error. –  Corbin Apr 7 at 21:34

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.