Stack implementation using vectors with no overflow version 1.0

Question

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");
}

Answer 1

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

Answer 2

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.