Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Please code review

#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;
}
share|improve this question

3 Answers

up vote 2 down vote accepted

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
share|improve this answer
up vote 3 down vote

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;.

share|improve this answer
While I see no fault in your argument, there are people that I know who thinks that form is easier to follow. – Jeff Mercado Sep 7 '11 at 7:12
@Jeff Mercado: well, if these people even have problems reading expressions like "1 == 2", I'm curious whether they can write code at all. – loki2302 Sep 7 '11 at 7:16
1  
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". – loki2302 Sep 7 '11 at 7:23
1  
@Jeff: Really? You know people who think introducing a ternary operator make the code easier to follow????? – Nathanael Sep 7 '11 at 16:41
1  
@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! – loki2302 Sep 7 '11 at 19:24
show 2 more comments
up vote 0 down vote
  • 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;
};
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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