Stack implementation using vectors and templates with no overflow version 1.3

Question

Below is the code for stack along with one or two extra options. Kindly let me know if there are any concerns/critical issues. Below are the links for previous versions.

Version 1.0

Version 1.1

Version 1.2

 //*************Version 1.3: Begineer's Implementation of Stack ***************//
#include <iostream>
#include <fstream>
#include <stdexcept>
template <class T>
class Mystack
{
private:
 T *input;
 int top;
 int capacity;
public:
 Mystack(); 
 Mystack(const Mystack<T> &source); 
 Mystack<T> & operator=(Mystack<T> source); 
 ~Mystack(); 
 void push(T const& x); 
 void pop(); 
 T& topElement() const; 
 bool isEmpty() const; 
 void print(std::ostream &os) const; 
 void swap(Mystack<T> &source); 
 friend std::ostream& operator<<(std::ostream& s, Mystack<T> const& d) //operator<< overloading
 {
 d.print(s);
 return s;
 }
};
template <class T>
Mystack<T>::Mystack() //default constructor 
{
 top = -1;
 capacity = 5;
 input = new T[capacity];
}
template <class T>
Mystack<T>::Mystack(const Mystack<T> &source) //copy constructor
{
 input = new T[source.capacity];
 top = source.top;
 capacity = source.capacity;
 for (int i = 0; i <= source.top; i++)
 {
 input[i] = source.input[i];
 }
}
/**COPY AND SWAP IDIOM**/
template <class T>
void Mystack<T>::swap(Mystack<T>& source) //swaps the content of 'source' to the object that calls it
{
 std::swap(input, source.input);
 std::swap(top, source.top);
 std::swap(capacity, source.capacity);
}
template <class T>
Mystack<T> & Mystack<T>::operator=(Mystack<T> source) // assignment operator overload 
{ //^^^^^ passing by value hence copy constructor gets invoked, 
 // thus no headache of copying, no code duplication, exception safe
 this->swap(source);
 return *this; //temporary holder 'source' will get destroyed, 'source' scope ends and its destructor is called,
} //avoiding memory leak 
template <class T>
Mystack<T>::~Mystack() // destructor
{
 delete[] input;
}
template <class T>
void Mystack<T>::push(T const& x) //Passing x by Const Reference 
{ // Valus of x cannot be changed now in the function!
 if (top + 1 == capacity)
 {
 T *vec = new T[capacity * 2];
 for (int i = 0; i <= top; i++)
 {
 vec[i] = std::move(input[i]);
 } 
 std::swap(input, vec);
 capacity *= 2;
 delete [] vec; // Avoiding Memory Leak.
 }
 input[++top] = x;
}
template <class T>
void Mystack<T>::pop() //pop the element from the top of stack
{
 if (isEmpty())
 {
 throw std::out_of_range("Stack Underflow");
 }
 /*else
 {
 std::cout << "The popped element is" << input[top--];
 }*/
 top--;
}
template <class T>
bool Mystack<T>::isEmpty() const //const: none of the class members can be modified in this function 
{
 return top == -1;
}
template <class T>
T& Mystack<T>::topElement() const // returns top element of the stack
{
 if (isEmpty())
 {
 throw std::out_of_range("No Element to Display");
 }
 else
 {
 //std::cout << "The top element is : " << input[top];
 return input[top];
 }
}
template <class T>
void Mystack<T>::print(std::ostream &os) const //a more of a general print function, can be used to write to a file 
{
 for (int i = 0; i <= top; i++)
 {
 os << input[i] << " ";
 }
}
int main()
{
 Mystack<int> intstack, inttemp;
 Mystack<float> floatstack, floattemp;
 Mystack <int> temp(intstack);
 Mystack<char> charstack, chartemp;
 int type_choice;
 std::ofstream some_file("testfile.txt"); // creation of file 
 int int_elem;
 float float_elem;
 char char_elem;
 int ch = 1;
 std::cout << "Enter the type of stack" << std::endl;
 std::cout << "1. int ";
 std::cout << "2. float ";
 std::cout << "3. Char" << std::endl;
 std::cin >> type_choice;
 std::cout << "\n1. Push ";
 std::cout << "2. Top ";
 std::cout << "3. IsEmpty ";
 std::cout << "4. Pop ";
 std::cout << "5. Write Data to a file";
 std::cout << "6. Copy Constrcutor Use";
 std::cout << "7. Assignemnt Operator Use";
 std::cout << "8. Print ";
 std::cout << "9. Exit" << std::endl;
 if (type_choice == 1)
 {
 int ch = 1; 
 while (ch > 0)
 {
 std::cout << "Enter the choice" << std::endl;
 std::cin >> ch;
 switch (ch)
 {
 case 1:
 std::cout << "Number to be pushed" << std::endl;
 std::cin >> int_elem;
 intstack.push(int_elem);
 break;
 case 2:
 try
 {
 std::cout << "Top Element is: " << intstack.topElement();
 }
 catch (std::out_of_range &oor)
 {
 std::cerr << "Out of Range error:" << oor.what() << std::endl;
 }
 break;
 case 3:
 std::cout << "Check Empty" << std::endl;
 if (intstack.isEmpty())
 std::cout << "Stack is Empty";
 else
 std::cout << "Stack is not Empty";
 break;
 case 4:
 std::cout << "Pop the element" << std::endl;
 try
 {
 intstack.pop();
 }
 catch (const std::out_of_range &oor)
 {
 std::cerr << "Out of Range error: " << oor.what() << '\n';
 }
 break;
 case 5:
 std::cout << "Stack data to file" << std::endl;
 intstack.print(some_file); //printing data to a file
 break;
 case 6:
 {
 Mystack<int> s4(intstack); // copy constructor called
 s4.print(std::cout);
 break;
 }
 case 7:
 inttemp = intstack; // assignment operator overload
 inttemp.print(std::cout);
 break;
 case 8:
 std::cout << intstack; // << operator overloading
 break;
 case 9:
 exit(0);
 default:
 std::cout << "Enter a valid input" << std::endl;
 break;
 }
 }
 }
 else if (type_choice == 2)
 {
 int ch = 1;
 while (ch > 0)
 {
 std::cout << "Enter the choice" << std::endl;
 std::cin >> ch;
 switch (ch)
 {
 case 1:
 std::cout << "Number to be pushed" << std::endl;
 std::cin >> float_elem;
 floatstack.push(float_elem);
 break;
 case 2:
 try
 {
 std::cout << "Top Element" << floatstack.topElement();
 }
 catch (std::out_of_range &oor)
 {
 std::cerr << "Out of Range error:" << oor.what() << std::endl;
 }
 break;
 case 3:
 std::cout << "Check Empty" << std::endl;
 if (floatstack.isEmpty())
 std::cout << "Stack is Empty";
 else
 std::cout << "Stack is not Empty";
 break;
 case 4:
 std::cout << "Pop the element" << std::endl;
 try
 {
 floatstack.pop();
 }
 catch (const std::out_of_range &oor)
 {
 std::cerr << "Out of Range error: " << oor.what() << '\n';
 }
 break;
 case 5:
 std::cout << "Stack data to file" << std::endl;
 floatstack.print(some_file); //data to file
 break;
 case 6:
 {
 Mystack<float> s5 = floatstack; // copy constructor called
 s5.print(std::cout);
 break;
 }
 case 7:
 floattemp = floatstack; // assignment operator overload
 floattemp.print(std::cout);
 break;
 case 8:
 std::cout << floatstack; //operator << overloading 
 break;
 case 9:
 exit(0);
 default:
 std::cout << "Enter a valid input" << std::endl;
 break;
 }
 }
 }
 else if (type_choice == 3)
 {
 int ch = 1;
 while (ch > 0)
 {
 std::cout << "Enter the choice" << std::endl;
 std::cin >> ch;
 switch (ch)
 {
 case 1:
 std::cout << "Number to be pushed" << std::endl;
 std::cin >> char_elem;
 charstack.push(char_elem);
 break;
 case 2:
 try
 {
 std::cout << "Top Element" << charstack.topElement();
 }
 catch (std::out_of_range &oor)
 {
 std::cerr << "Out of Range error:" << oor.what() << std::endl;
 }
 break;
 case 3:
 std::cout << "Check Empty" << std::endl;
 if (charstack.isEmpty())
 std::cout << "Stack is Empty";
 else
 std::cout << "Stack is not Empty";
 break;
 case 4:
 std::cout << "Pop the element" << std::endl;
 try
 {
 charstack.pop();
 }
 catch (const std::out_of_range &oor)
 {
 std::cerr << "Out of Range error: " << oor.what() << '\n';
 }
 break;
 case 5:
 std::cout << "Stack data to file" << std::endl; 
 charstack.print(some_file); //printing stack data to file
 break;
 case 6:
 {
 Mystack<char> s6 = charstack; // copy constructor called
 s6.print(std::cout);
 break;
 }
 case 7:
 chartemp = charstack;
 chartemp.print(std::cout);
 break;
 case 8:
 std::cout << charstack; //operator << overloading
 break;
 case 9:
 exit(0);
 default:
 std::cout << "Enter a valid input" << std::endl;
 break;
 }
 }
 }
 else
 std::cout << "Invalid Choice";
 std::cin.get();
}

Answer 1

Don't like this comment:

{ // Valus of x cannot be changed now in the function!

I think that is obvious. But you are not manipulating x in the function so not a problem and you make a copy when you put it on the stack.

Don't like the usless else

else
{
 //std::cout << "The popped element is" << input[top];
 --top;
}

The true part of the branch never returns. So the else is redundant. I like to keep validation logic (test that the pre-conditions are met) separate from the business logic of the class.

Apart from that I can not spot anything wrong.

Couple of additions (I mention in the first review).

• Using placement new/delete so that you don't construct objects that are not needed.

• You could add move semantics to the class

  • Constructor
  • assignment
  • push.

Answer 2

in your push you allocate the storage with new T[] so you should delete with delete[]

template <class T>
void Mystack<T>::push(T const& x) //Passing x by Const Reference 
{ // Valus of x cannot be changed now in the function!
 if (top + 1 == capacity)
 {
 T *vec = new T[capacity * 2];
 for (int i = 0; i <= top; i++)
 {
 vec[i] = std::move(input[i]);
 } 
 std::swap(input, vec);
 capacity *= 2;
 delete[] vec; // Avoiding Memory Leak.
 }
 input[++top] = x;
}

consider also adding a moveable push that you can call with vector.push(std::move(foo));

Your pop doesn't, looks like a typo during debugging:

template <class T>
void Mystack<T>::pop() //pop the element from the top of stack
{
 if (isEmpty())
 {
 throw std::out_of_range("Stack Underflow");
 }
 else
 {
 //std::cout << "The popped element is" << input[top];
 --top;
 }
}

Besides that because you allocate with new T[] the default constructor will be called for each element each time you allocate and the destructor each time you delete[]. To avoid that you can use placement new and placement delete instead. This requires you to loop over all elements that you initialized and call the destructor on it during deallocation. Though this is more complicated to keep track of; the std containers outsource this to an allocator object.

Answer 3

There is two notes I have regarding your code: Use unsigned more and the use of -1 in the index variable - it screams "invalid offset" to me, even though I know you check for it.

You current semantics of top is "-1 means empty, otherwise it is the index of top element". If you change it to "index of the next element to be added", 0 will have the meaning of empty stack, allowing you to use unsigned type for top.