patterncppMinor
Stack implementation using vectors with no overflow version 1.0
Viewed 0 times
vectorsstackwithoverflowversionusingimplementation
Problem
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 0)
{
std::cout > ch;
switch (ch)
{
case 1:
std::cout > 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");
}Solution
-
What is
-
Any member function that doesn't modify any data members should be
One example is
-
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
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
What you should do is have the function return
-
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
-
One case where output is really not needed is in
-
It's important to know that
-
Be aware that
-
If you'd like for this stack class to handle types other than
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.Code Snippets
int Mystack::topElement() const
{
std::cout << "The top element is : " << A[top];
return A[top];
}Mystack::~Mystack()
{
delete[] A;
}return (top == -1);Context
StackExchange Code Review Q#75235, answer score: 3
Revisions (0)
No revisions yet.