4
\$\begingroup\$

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

 #include"iostream"
    #define MAX_SIZE 5
    using namespace std;
    class Mystack
    {
    private:
        int A[MAX_SIZE];
        int top;
    public:
        Mystack();
        void push(int x);
        void pop();
        void topElement();
        void print();
        void isEmpty();
    };
        Mystack::Mystack()
        {
            top = -1;
        }
        void Mystack::push(int x)
        {
            if(top == MAX_SIZE-1)
            {
                cout << "Stack Overflow"<< endl;
                return;
            }
            else
            {
                top++;
                A[top] = x;
            }
        }
        void Mystack::pop()
        {
            if (top != -1)
            {
                top--;
            }
            else
            {
                cout << "Stack Underflow" << endl;
                return;
            }
        }
        void Mystack::isEmpty()
        {
            if (top == -1)
            {
                cout << "Is Empty"<< endl;
            }
            else
            {
                cout << "Not Empty" << endl;
            }
        }
        void Mystack::topElement()
        {
            cout<<"The top element is : "<< A[top];
        }
        void Mystack::print()
        {
            for (int i = 0; i <=top; i++)
            {
                cout << "top:  " << A[i]<< endl;
            }
        }
    void main()
    {
        Mystack s1;
        int num,ch = 1;
        while (ch >0)
        {
            cout << "\n1. PUSH" << endl;
            cout << "2. TOP" << endl;
            cout << "3. IsEmpty" << endl;
            cout << "4. POP" << endl;
            cout << "5. EXIT" << endl;
            cout << "6. Print" << endl;
            cout << "Enter the choice"<<endl;
            cin >> ch;
            switch (ch)
            {
            case 1:
                cout << "Enter the number to be pushed" << endl;
                cin >> num;
                s1.push(num);
                break;
            case 2:
                cout << "Get the TOP Element" << endl;
                s1.topElement();
                break;
            case 3:
                cout << "Check Empty" << endl;
                s1.isEmpty();
                break;
            case 4:
                cout << "POP the element" << endl;
                s1.pop();
                break;
            case 5: exit(0);
            case 6:
                s1.print();
                break;
            }
        }
        system("pause");
    }
\$\endgroup\$
1

3 Answers 3

Reset to default
9
\$\begingroup\$

This review is in response to the request for help on "overall coding practice." It does not delve into the syntax or semantics of the C++ language. There are three levels: Computer Science, Architecture, Variable Names.

Computer Science

Technically the implementation is not a stack because:

myStack.push(4);
myStack.pop();

does not return 4. To be a proper stack, rather than returning void, pop should be:

int pop();

In other words, pop() must return something that was pushed onto the stack (or an error, but that's an architectural issue not a principle of computer science).

Architecture

Strictly speaking topElement() could return a void and write a message to the console, but it is conventional for it to return something of the same type as the top element of the stack - in the case of reference types, the question of whether it should be a copy [and what type of copy] or a reference to the object itself muddies the waters.

The current implementation of void pop(); points to a larger architectural issue, a low degree of modularity. Currently, user interface is spread across the code and the stack data structure even writes messages to the console. The effect:

  • The implementation of stack is not broadly reusable.
  • The user interface does not have a clear design and would be hard to debug at a larger scale. There's a bit of the "spaghetti design pattern."

A more modular approach to implementing stack might be:

public:
    Mystack();
    void push(int x);
    int pop();
    int topElement();
    bool isEmpty();
    bool isFull();  // added to allow bounds checking
    void print();   // could be handled elsewhere but that's pure opinion.

and placing - from the viewpoint of the stack - all the user interface code in Main. Whether the user interface logic all lives there or in it's own class/module is another question. But reusability and maintenance certainly offer suggestions.

For example, breaking out the user interface and its logic and its content into modules allows handling nasty issues like multiple languages and the quirks of UniCode to be handled more gracefully. String output is the one of the places Your-Not-Gonna'-Need-It [YNGNI] fails and why it is a baked in module for so many development frameworks.

Variable Names

A, num, ch, and s1 though perhaps conventional for C++ example code, don't really scale well because they lack information. Even in the original code:

cout << "top:  " << A[i]<< endl;

is more than 50 lines from:

private:
    int A[MAX_SIZE];

and that distance is only likely to grow as a useful program adds features.

Final Thought

I cannot help but recommend McConnell's Code Complete as a guide to deepening one's understanding of the ways in which software can be organized.

\$\endgroup\$
8
  • \$\begingroup\$ int Mystack::pop() { if (top != -1) { cout << "The popped element is" << A[top]; return A[top--]; } else { cout << "Stack Underflow" << endl; } return 0; } Is this correct implementation of POP function Since now I have used int pop() instead of void pop(). Do I need to catch and print the popped element in my MAIN or printing in the int pop() function will suffice as long as I am returning the popped element values to MAIN. \$\endgroup\$ Commented Dec 30, 2014 at 15:20
  • \$\begingroup\$ I don't know how to format code in comment section. Kindly bear with me. Many Thanks! \$\endgroup\$ Commented Dec 30, 2014 at 15:21
  • \$\begingroup\$ @Unbreakable comments only support inline code. No linebreaks, sorry. \$\endgroup\$ Commented Dec 30, 2014 at 19:35
  • \$\begingroup\$ @Unbreakable The issue I see is that instead of throwing an exception when pop() is called on an empty stack, 0 is returned and there is no way to tell programmatically if the 0 is an error or just the result of push(0)` having been called previously. There is no guarantee that cout goes to a terminal and not to a serial or null device since cout sends its output to stdout and stdout may be redirected. cplusplus.com/reference/cstdio/stdout \$\endgroup\$ Commented Dec 30, 2014 at 22:25
  • 2
    \$\begingroup\$ Note: the C++ standard chose to do the same thing as Unbreakable regarding pop/top. They chose to have top() return the topmost value and pop pop it without returning because doing so avoided the need to do extra copies of objects. C++ did not have move mechanics back then. \$\endgroup\$ Commented Dec 30, 2014 at 23:26
4
\$\begingroup\$

Comments

You have using namespace std before the class declaration, normally you would place that in the .cpp or even better using statement in the individual functions where they are needed.

Initialize all member variables using an initialization list

MyStack() : A{0}, top(-1) {  }

Your pop function just removes the top most int, but normally one would like to get the value as well not only remove it. int pop() seems more natural.

Your functions do not deliver any error codes to the caller instead you select to display error message with cout. It makes the class less versatile, instead you should let the user of your class handle the error messages and just return error code/throw exception.

It is good to be consistent with {} for each if statement not switch between with and without

e.g.

 if (top == -1)
 {
   cout << "Is Empty"<< endl;
 }
 else
   cout << "Not Empty" << endl;

EDIT

Your functions do not deliver any error codes to the caller instead you select to display error message with cout. It makes the class less versatile, instead you should let the user of your class handle the error messages and just return error code/throw exception.

with less versatile I mean that if your class does not throw exceptions the user of the class cannot take any corrective action if something goes wrong or give out custom error messages in his own language.

\$\endgroup\$
5
  • \$\begingroup\$ I understood the points you made except one. Can you please explain what do you mean by Your functions do not deliver any error codes to the caller instead you select to display error message with cout. It makes the class less versatile, instead you should let the user of your class handle the error messages and just return error code/throw exception. Can you give one example or something, or use any chunk of my code. Many Thanks! \$\endgroup\$ Commented Dec 30, 2014 at 14:03
  • 3
    \$\begingroup\$ @Unbreakable In C++, the preferred way of handling errors is by throwing an exception [see: msdn.microsoft.com/en-us/library/hh279678.aspx]. Exceptions allow for information to be passed up the call stack. Writing to the console, might alert a user that there was a problem, but many times there is no user [e.g. a server] or even when there is, there are many lines of code and no way of pinning down the source of a message on the console. \$\endgroup\$ Commented Dec 30, 2014 at 14:40
  • 1
    \$\begingroup\$ @Unbreakable i edited my reply to explain what I meant. \$\endgroup\$ Commented Dec 31, 2014 at 21:22
  • \$\begingroup\$ @CyberSpockI tried exception handling in this code \$\endgroup\$ Commented Dec 31, 2014 at 22:02
  • \$\begingroup\$ @Unbreakable i didn't see any exception handling in the code you posted in this question. \$\endgroup\$ Commented Jan 1, 2015 at 0:24
2
\$\begingroup\$

And it does not seem to be right at a first glance: When inserting (push), the top will be incremented before inserting the element, when calling pop, the top will be decremented first. The pop - method should return the element in the array, but in this case the element would not be the top of the stack.
Also you should use a template parameter int for the stack size , such as
class Mystack<int SIZE>, to make the size of it dynamic rather than using a define.

\$\endgroup\$
6
  • \$\begingroup\$ 'top' has to get incremented first, because it has been initialized with -1. If I don't increment it first then A[-1] will hold the first element which is illegal. Same goes with the POP. Am I making sense here. Kindly let me know if I am wrong. Thanks! \$\endgroup\$ Commented Dec 30, 2014 at 13:58
  • \$\begingroup\$ Like I said this is related to using pop with a return of the top element. So if you increment to 0, e.g. , and insert an element, than pop you would be at the illegal index , too. But this is more related to the stack implementations I use, which do this pop - and - return, which has its own impacts, of course, if you just want to examine it. \$\endgroup\$ Commented Dec 30, 2014 at 14:20
  • \$\begingroup\$ Yes, You are correct. Anyways I also want to implement int pop() instead of void pop(). \$\endgroup\$ Commented Dec 30, 2014 at 15:12
  • \$\begingroup\$ And this is the code I came up with. int Mystack::pop() { if (top != -1) { cout << "The popped element is" << A[top]; return A[top--]; } else { cout << "Stack Underflow" << endl; } return 0; } \$\endgroup\$ Commented Dec 30, 2014 at 15:12
  • \$\begingroup\$ I don't know how to format code in "comment section". Kindly bear with me. \$\endgroup\$ Commented Dec 30, 2014 at 15:12

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.