2
\$\begingroup\$

This is my first time using C++ Templates to make a data structure. I have written a program to implement a stack of up to 5 items.

#include<iostream>
#include<iomanip>

using namespace std;

template<class T>
class Queue{
private:
    T arrays[5];
    int i; //empty stack
public:
    Queue()
    {
        arrays[5] = {0}; //Empty stack
        i = -1; //Setting stack to be empty
    }
    T PUSH(T item)
    {
        if(i == 4)
        {
            cout << "Stack is full" << endl;
        }
        else
        {
        i++;
        arrays[i] = item;
        }
    }
    T POP()
    {
        if(i == -1)
        {
            cout << "Stack is empty" << endl;
        }
        else
        {
        cout << arrays[i] << " has been popped" << endl;
        i--;

        }
    }
    T TOP()
    {
        if(i == -1)
        {
            cout << "Stack is empty" << endl;
        }
        else
        {
        cout << "Element at top of stack is " << arrays[i] << endl;
        }
    }

};


int main()
{
    Queue <int> Q;
    int option,push;
    do{
    cout << "Select any of the following option" << endl;
    cout << "1.Push\n2.Pop\n3.Top\nPress any other button to exit" << endl;
    cin >> option;

    switch(option)
    {
        case 1:
            cout << "Which element to push" << endl;
            cin >> push;
            Q.PUSH(push);
            break;
        case 2:
           Q.POP();
            break;
        case 3:
           Q.TOP();
            break;
        default:
            cout << "Error!" << endl;
            break;

    }
    }while(option < 4);
return 0

}

Though I have got the program working. I would like to ask if there is any way I can improve on the code.

\$\endgroup\$
  • \$\begingroup\$ I have got the program working. That's strange I see problems that would cause undefined behavior. \$\endgroup\$ – R Sahu Feb 24 '16 at 5:06
  • 1
    \$\begingroup\$ PUSH, POP and TOP say that they return a T, but they actually do not. You should consider turning on warnings and "treat warnings as errors" to make the compiler check these things for you. \$\endgroup\$ – nwp Feb 24 '16 at 11:40
1
\$\begingroup\$

Queue != Stack

Queue behaviour is FIFO. Stack behaviour is LIFO. Currently your datastructure is called Queue, but it implements Stack, has stack error messages etc. This is very confusing.

Magic Numbers

There are three main numbers you're using for manipulating your stack 5, 4 and -1. Out of context, it's not immediately obvious what they mean. Replacing them with constants would make the code easier to read.

Undefined behaviour

As has been hinted at by @Quuxplusone, this:

_myarr[5]={0};

Isn't doing what you're expecting it to. Rather than initializing all 5 of the elements in the array, which is what I suspect you expected, it is attempting to initialize the element with index 5. Since arrays are index from 0 and it only contains 5 elements, you're overflowing the array. This may or may not work, depending on how lucky you get.

Return Values

You've declared PUSH,POP,TOP all returning type T, however you're never returning anything. Depending on your compiler + switches this may or may not actually compile. PUSH probably doesn't need to return the value it's added anyway...

Console Output

Console output is great for testing your code to see if it's doing what you're expecting, however writing it into your classes gives the console access to private data members. Try to avoid putting console interaction in your data structures. If you had written the console interactions outside of the class, you would have noticed that POP etc didn't return values a lot earlier in the cycle.

Error Conditions

If I pop from an empty stack, you write to the console, however the caller has no way to know that they haven't retrieved a value. Worse, if I push onto a full stack you simply throw the value away. You should notify the user that there has been a failure, possibly by throwing an exception.

Exit

"Any other button" isn't a great hint that you need to press 4 or above to get out of the application. Help your users, be explicit.

using namespace::std

Is generally frowned on, be explicit.

Indentation

Indentation matters, be consistent, it's a lot easier to read.

Putting it all together

I'm sure there's still room for improvement, however combining all of the above, the Stack would look a bit more like this:

#include<iostream>
#include<iomanip>
#include <exception>


template<class T>
class Stack {
private:
    static const int stack_size = 5;
    static const int stack_full = stack_size - 1;
    static const int stack_empty = -1;

    T arrays[stack_size] = {};
    int i = stack_empty;
public:
    void PUSH(T item)
    {
        if (i == stack_full)
        {
            throw std::runtime_error{ "Stack is full" };
        }

        i++;
        arrays[i] = item;
    }
    T POP()
    {
        if (i == stack_empty)
        {
            throw std::runtime_error{ "Stack is empty" };
        }

        T &removed = arrays[i];
        i--;
        return removed;
    }
    T TOP()
    {
        if (i == stack_empty)
        {
            throw std::runtime_error{ "Stack is empty" };
        }

        return arrays[i];
    }

};


int main()
{
    Stack <int> Q;
    int option, push;
    do {
        std::cout << "Select any of the following option" << std::endl;
        std::cout << "1.Push\n2.Pop\n3.Top\n4.Exit" << std::endl;
        std::cin >> option;

        try {
            switch (option)
            {
            case 1:
                std::cout << "Which element to push" << std::endl;
                std::cin >> push;
                Q.PUSH(push);
                break;
            case 2:
            {
                int removed = Q.POP();
                std::cout << removed << " has been popped" << std::endl;
                break;
            }
            case 3:
            {
                int top = Q.TOP();
                std::cout << "Element at top of stack is " << top << std::endl;
                break;
            }
            case 4:
                break;
            default:
                std::cout << "Error!" << std::endl;
                break;
            }
        }
        catch (std::runtime_error error) {
            std::cout << error.what() << std::endl;
        }
    } while (option < 4);
}
\$\endgroup\$
0
\$\begingroup\$
arrays[5] = {0};

Wow. This was literally the exact example in a blog post I read today.

_myarr[5]={0};

The crazy thing is that that blog post was written in March 2014. Spooky.

Anyway, I don't have time for a real review right now, but wow, that's just crazy coincidental. Also, that's your bug.

\$\endgroup\$
  • \$\begingroup\$ Please expand on how that observation constitutes a bug in OP's code, and you'll have a good CR answer. \$\endgroup\$ – Mathieu Guindon Mar 3 '16 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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