-3
\$\begingroup\$

My CS teacher sent me the following code for implementing the stack. I've been programming for a few years in higher-level languages, but have rather weak experience with C++. I mean, this code just doesn't seem right to me. What is done in a bad way here and what would you improve?

#include <iostream>

using namespace std;

struct stackElem

{
    stackElem* nxt;

    int dana;
};

bool isEmpty(stackElem* S)

{
    return !S;
}

stackElem* top(stackElem* S)

{
    return S;
}

void add(int value, stackElem** S)

{
    stackElem* elem = new stackElem;

    elem->dana = value;

    elem->nxt = *S;

    *S = elem;
}

void get(stackElem** S)

{
    if (*S)

    {
        stackElem* elem = *S;

        *S = (*S)->nxt;

        delete elem;
    }
}

int main()

{
    stackElem* S = NULL;

    int i;

    // fill the stack
    for (i = 1; i <= 10; i++)

    {
        add(i, &S);
    }

    while (!isEmpty(S))

    {
        cout << top(S)->dana << endl;

        get(&S);
    }

    return 0;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review Stack Exchange! Unfortunately, 'what is wrong with this' type of questions are not allowed. This site is for people who want a critique of working code that they have written. \$\endgroup\$ – Rish May 14 at 19:23
  • \$\begingroup\$ Sure. Changed my question. \$\endgroup\$ – throwaway142 May 14 at 20:45
  • 2
    \$\begingroup\$ Unfortunately the title is implying that the code isn't working which is going to cause people here to think it's of topic and vote to close... \$\endgroup\$ – Emily L. May 14 at 21:58
  • 3
    \$\begingroup\$ "My CS teacher sent me the following code..." Code Review will only review code posted by the author (or maintainer) of the code. While the code may be working, if this is not your code, you cannot ask for a review of it here. \$\endgroup\$ – AJNeufeld May 14 at 22:04
  • \$\begingroup\$ (FWIW, I don't see C++ code but for new&delete. Just about everything is wrong: this got to be intentional.) \$\endgroup\$ – greybeard May 15 at 1:38
2
\$\begingroup\$

This code looks more like C than C++. The only C++ feature used is cout. While the standard C++ library already provides std::stack, I assume the idea is to implement a stack from scratch to show how that would work. With that in mind, here's what I would change to make it better C++ code:

Create a class representing a stack

The code as it is only defines a single stack element, and has global functions that manipulate pointers to elements. In C++, you would create a class to manage everything related to the stack:

class Stack {
    struct Element {
         Element *next;
         int value;
    };

    Element *top = nullptr;

public:
    ~Stack() {
         // Write code here to clean up the stack
    }

    bool isEmpty() const {
         return top == nullptr;
    }

    void push(int value) {
         // Create a new Element and make it the new top
    }

    // other functions here
    ...
};         

This hides the management of stack elements, forcing the rest of the code to use the public functions to manipulate the stack, making it harder to make mistakes. You would use it like so:

int main() {
    Stack S;

    for (int i = 1; i <= 10; ++i)
         S.push(i);

    while (!S.empty()) {
         std::cout << S.top() << '\n';
         S.get();
    }
}

Consider making a pop() function that combines top() and get()

This would be a logical mirror of the push() function, and allows writing a bit shorter code, like:

    while (!S.empty())
         std::cout << S.pop() << '\n';

Make the class a template

One big issue with your version of a stack is that it can only store ints. What if you want a stack that stores floats, or strings, or anything else? You want to avoid having to write different versions of the stack just to handle different types. This is what templates are made for. It's quite easy to make a template:

template<typename T>
class Stack {
    struct Element {
        Element *next;
        T value;
    };

    ...

    void push(T value) {
        ...
    }

    T get() {
        return top->value;
    }

    ...
};

And then you can use it like:

int main() {
    Stack<int> S;

    for (int i = 1; i <= 10; ++i)
         S.push(i);

    ...
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Sometimes it is better to not answer a question, especially when there are already comments that it is off-topic. \$\endgroup\$ – pacmaninbw May 15 at 19:20

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