1
\$\begingroup\$

Please review this:

#include <iostream>
#include <vector>
#include <cassert>
using namespace std;

template <class T>
class Stack
{
public:
  Stack() {}
  void push(T x);
  void pop();
  T & top();
  size_t size();
  bool empty();
private:
  std::vector<T> elems;
};

template <class T>
void Stack<T>::push(T x)
{
  elems.push_back(x);
}

template <class T>
void Stack<T>::pop()
{
  if (elems.size() > 0)
    {
      elems.erase(elems.end() - 1);
    };
}

template <class T>
T & Stack<T>::top()
{
  assert(elems.size() > 0);
  return elems.at(elems.size()-1);
}

template <class T>
size_t Stack<T>::size()
{
  return elems.size();
}

template <class T>
bool Stack<T>::empty()
{
  return elems.size() == 0 ? true : false;
}

int main()
{

  Stack<int> s;
  s.push(1);
  s.push(2);
  s.push(3);
  s.push(4); 
  s.push(5);

  cout << "size: " << s.size() << endl;
  cout << "top element: " << s.top() << endl;

  s.pop();
  s.pop();

  cout << "size: " << s.size() << endl;
  cout << "top element: " << s.top() << endl;
  cout << "empty: " << s.empty() << endl;

  s.pop();
  s.pop();
  s.pop();

  cout << "size: " << s.size() << endl;
  //cout << "top element: " << s.top() << endl;
  cout << "empty: " << s.empty() << endl;

  return 0;
}
\$\endgroup\$
2
\$\begingroup\$

I agree with loki on the size() method.

Maybe assert is not the correct thing here.

template <class T>
T & Stack<T>::top()
{
  assert(elems.size() > 0);
  return elems.at(elems.size()-1);
}

Maybe an exception would be a better idea.
That way the user has the potential to recover but if they don't specifically check then the application still exits like assert. Also asserts can be turned of. So there is the potential that production code you would get illegal access to the underlying vector.

Why not have the same test on pop() rather than silently failing in this situation?

template <class T>
void Stack<T>::pop()
{
  if (elems.size() > 0)  // This causes a silent fail
                         // But is the same kind of programming bug as top
\$\endgroup\$
3
\$\begingroup\$

Most of the cases I wrote for your other question are also applicable here. Here's an extra thing:

return elems.size() == 0 ? true : false;

The expression elems.size() == 0 is of type bool. What you write is like if(true) return true; else return false;.

\$\endgroup\$
  • \$\begingroup\$ While I see no fault in your argument, there are people that I know who thinks that form is easier to follow. \$\endgroup\$ – Jeff Mercado Sep 7 '11 at 7:12
  • \$\begingroup\$ @Jeff Mercado: well, if these people even have problems reading expressions like "1 == 2", I'm curious whether they can write code at all. \$\endgroup\$ – Andrey Agibalov Sep 7 '11 at 7:16
  • 1
    \$\begingroup\$ Coding-styling aspects of this kind is often really bad idea. If someone hires me to write C++ code, I'll write C++ code. If they want me to use a sub-set of C++ (like, no templates) or they require me to write stupidities (like if(a == true) instead of if(a) in case a is bool) that's not C++, that's "let's play the game our teamlead/PM/any-other-idiot-who-is-bored invented". \$\endgroup\$ – Andrey Agibalov Sep 7 '11 at 7:23
  • 1
    \$\begingroup\$ @Jeff: Really? You know people who think introducing a ternary operator make the code easier to follow????? \$\endgroup\$ – Nathanael Sep 7 '11 at 16:41
  • 1
    \$\begingroup\$ @Nathanael: it's something with their perception of the world. I'm not a native English speaker, but when I see x == 0, I read it as "x is zero", and when I see return x == 0, I read it as "tell them that x is zero". Ternary in return x == 0 ? true : false is like "tell them yes if x is zero and no if it's not". Too many words! \$\endgroup\$ – Andrey Agibalov Sep 7 '11 at 19:24
0
\$\begingroup\$
  • Methods size() and empty() should be declared const. There also should be overloaded top() const method.
  • Instead of elems.at(elems.size()-1) use elems.back(). There is also a method pop_back().
  • Don't use using namespace std, especially in header files.
  • Implementation of these methods is really small, so I would recommend definitions inside class declaration.
  • It would be nice to have a constructor that takes expected size of the stack and reserve that amount in elems.
  • With assert() the prerequisite will be unchecked in Release mode – use exception.

#include <vector>
#include <stdexcept>

template <class T>
class Stack {
public:
    Stack(size_t size = 0) {
        elems.reserve(size);
    }

    void push(T x) {
        elems.push_back(x);
    }

    void pop() {
        if (!empty()) {
            elems.pop_back();
        }
    }

    T& top() {
        if (empty()) {
            throw std::out_of_range("Stack is empty");
        }
        return elems.back();
    }

    T top() const {
        if (empty()) {
            throw std::out_of_range("Stack is empty");
        }
        return elems.back();
    }

    size_t size() const {
        return elems.size();
    }

    bool empty() const {
        return size() == 0;
    }

private:
    std::vector<T> elems;
};
\$\endgroup\$

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.