5
\$\begingroup\$

This is an implementation of a stack where I have used vectors. Can anyone please review it and let me know how I can improve this code and overall coding practice?

    #include"iostream"
class Mystack
{
private:
    int *A;
    int top;
    int size;
public:
    Mystack(int capacity);
    void push(int x);
    int pop();
    int topElement();
    void print();
    bool isEmpty();
};
Mystack::Mystack(int capacity)
{
    top = -1;
    size = capacity;
    A = new int[capacity];
}
void Mystack::push(int x)
{

    if (top + 1 == size)
    {
        int *vec = new int[size + size];
        for (int i = 0; i < size; i++)
        {
            vec[i] = A[i];
        }
        A = vec;
        size = size * 2;
        top++;
        A[top] = x;
        return;
    }
    else
    {
        top++;
        A[top] = x;
    }
}
int Mystack::pop()
{
    if (isEmpty())
    {
        std::cout << "Stack Underflow" << std::endl;
    }
    else
    { 
        std::cout << "The popped element is" << A[top];
        return A[top--];
    }
    return 0;
}
bool Mystack::isEmpty()
{
    if (top == -1)
    {
        std::cout << "Is Empty" << std::endl;
        return true;
    }
    else
    {
        std::cout << "Not Empty" << std::endl;
        return false;
    }
}
int Mystack::topElement()
{
    std::cout << "The top element is : " << A[top];
    return A[top];
}
void Mystack::print()
{
    for (int i = 0; i <= top; i++)
    {
        std::cout << A[i] << " ";
    }
}
void main()
{
    Mystack s1(10);
    int num, ch = 1;
    while (ch >0)
    {
        std::cout << "\n1. PUSH" << std::endl;
        std::cout << "2. TOP" << std::endl;
        std::cout << "3. IsEmpty" << std::endl;
        std::cout << "4. POP" << std::endl;
        std::cout << "5. EXIT" << std::endl;
        std::cout << "6. Print" << std::endl;
        std::cout << "Enter the choice" << std::endl;
        std::cin >> ch;
        switch (ch)
        {
        case 1:
            std::cout << "Enter the number to be pushed" << std::endl;
            std::cin >> num;
            s1.push(num);
            break;
        case 2:
            std::cout << "Get the TOP Element" << std::endl;
            s1.topElement();
            break;
        case 3:
            std::cout << "Check Empty" << std::endl;
            s1.isEmpty();
            break;
        case 4:
            std::cout << "POP the element" << std::endl;
            s1.pop();
            break;
        case 5: exit(0);
        case 6:
            s1.print();
            break;
        }
    }
    system("pause");
}
\$\endgroup\$
4
  • \$\begingroup\$ I have just seen your previous stack implementation, and hardly any of the suggestions were applied. Not making these changes and having us provide the same points is a waste of time for all of us. \$\endgroup\$
    – Jamal
    Commented Dec 30, 2014 at 18:12
  • \$\begingroup\$ Previous one was implementation with the use of arrays. This one is using vectors with no size limit. \$\endgroup\$ Commented Dec 30, 2014 at 18:25
  • \$\begingroup\$ And I did take care of some of the suggestions and trying to make the rest. Original Code Revised Code . Changes made 1. Removed: using namespace and used std 2. Use of function isEmpty() with bool value 3. Implemented function isFull 4. Used isEmpty() inside function in POP() 5. Used isFull() function inside Push() and a couple more. \$\endgroup\$ Commented Dec 30, 2014 at 18:26
  • \$\begingroup\$ Some of the suggestions were like reading one book to understand how to code properly and I am looking forward to read that book. I don't intend to waste anyone's time. Anyways thanks for the suggestion below. I will try to incorporate those. \$\endgroup\$ Commented Dec 30, 2014 at 18:27

2 Answers 2

3
\$\begingroup\$
  • What is A? You shouldn't use single-character variables as they're very ambiguous. You may know what it's for, but it doesn't mean anyone else does.

  • Any member function that doesn't modify any data members should be const.

    One example is topElement():

    int Mystack::topElement() const
    {
        std::cout << "The top element is : " << A[top];
        return A[top];
    }
    
  • This class performs memory allocation, but you never deallocate it, making it one big memory leak for anyone using it.

    You should have a destructor that uses delete to free this memory:

    Mystack::~Mystack()
    {
        delete[] A;
    }
    

    With a destructor in place, you should also have a copy constructor and assignment operator defined, as per the Rule of Three. It is especially important to define your own since the default ones perform a shallow copy, which shouldn't be done when you have pointers as data members. Those will instead be copied instead of the data they point to, which will mess up your data.

  • Although pop() "reports" underflows, it can still break since it doesn't handle errors properly. Specifically, it'll return 0 if the stack is already empty. Since 0 is a value, the calling code can store it like normal. So, pop() can be called repeatedly and it will always return this value.

    What you should do is have the function return void so that it won't have to return anything if the stack is empty. This is also how it's done in the standard library, in case you weren't aware.

  • Something like this shouldn't be displaying simple output, though it may be okay if you're just testing the functions. Otherwise, it should be done in main(), while the bool values should determine which messages should be displayed.

  • One case where output is really not needed is in isEmpty(). A simple function like this can just be done in one line:

    return (top == -1);
    
  • It's important to know that main() should return int. Some compilers allow it to return void, but this isn't allowed by the standard. Regardless, you should change it to return int.

  • Be aware that system("PAUSE") is Windows-specific thus non-portable. If you'd like to do a similar pause that is portable, use something like std::cin.get().

  • If you'd like for this stack class to handle types other than int, then consider using templates. There are many resources on this, and it should be worthwhile to learn about this.

\$\endgroup\$
2
  • \$\begingroup\$ Can you please tell me what your fifth point means? \$\endgroup\$ Commented Dec 31, 2014 at 5:22
  • 2
    \$\begingroup\$ @Unbreakable: That was also something mentioned in another answer to one of your other questions. A class like this shouldn't handle output; the calling code should. By returning a value like bool, you can determine what should be displayed. \$\endgroup\$
    – Jamal
    Commented Dec 31, 2014 at 5:24
3
\$\begingroup\$

Correct me wherever I am wrong. Some inputs:

  • If capacity == 0 ? Handle edge cases.
  • If capacity == INFINITE_VALUE ? Heap memory is finite. Handle exceptions say bad_alloc.
  • Memory Leak. A = vec? Free the previously allocated memory in A and then reassign the value.
  • Could we simplify std::cout to just cout?
  • topElement(). I haven't inserted anything but I wish to see the topElement. So A[-1] is unpredictible.
  • Have a default case to atleast alert the user of an incorrect input. Rest is all taken care by Jamal.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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