The general rule (at least for user codebases) is "make everything explicit"make everything explicit unless you have some specific reason that it needs to be implicit." So you should make your node constructor explicit as well.
The general rule (at least for user codebases) is "make everything explicit unless you have some specific reason that it needs to be implicit." So you should make your node constructor explicit as well.
The general rule (at least for user codebases) is "make everything explicit unless you have some specific reason that it needs to be implicit." So you should make your node constructor explicit as well.
#include <stdexcept>
And either <utility> or <algorithm>, for std::swap.
stack(int max) {
I very strongly recommend you make this constructor explicit. Right now it's a non-explicit (implicit) conversion, which means you're permitting your users to convert integers to stacks implicitly:
void foo(stack<int> s);
void bar() {
foo(42); // compiles and calls foo with stack<int>(42)
}
The general rule (at least for user codebases) is "make everything explicit unless you have some specific reason that it needs to be implicit." So you should make your node constructor explicit as well.
(I say "at least for user codebases" because the STL itself doesn't follow that principle — partly because the principle wasn't understood in 1998, and partly due to differences of opinion among the authors. "STL style" would be to make stack(int) explicit because it's a one-argument constructor, but leave all zero- and two-argument constructors implicit. I recommend against doing that.)
int max = -1; // -1 so isFull() == false when default constructor used
This comment doesn't help me understand. I would understand INT_MAX, but -1 just looks like it's breaking the class invariant enforced by the one-argument constructor:
if (max <= 0) throw std::out_of_range("stack size must be > 0");
Looking at these two lines in isolation, actually, I briefly wondered "wait, doesn't that always throw? You don't initialize max before that if, so it would have its default value, which is less than zero..." But then I realized that the name max here is overloaded: the max in the if statement is testing the function parameter, not the data member.
To eliminate confusion about name overloading, I strongly recommend that you sigil each of your data members with a trailing underscore: max_, size_, head_.
Then you don't have to write this-> when you access a member:
this->max = max;
can become just
max_ = max;
You don't need the disambiguating this->, because there's no confusion anymore about which max/max_ you mean.
stack(stack const& rhs) :
head(copyList(rhs.head)),
size(rhs.size),
max(rhs.size) {}
Do you see the typo?
Write some unit tests for your code! In particular, now that you've spotted a bug, write a unit test that would have caught this bug and commit it as part of the bugfix. That's called a "regression test."
stack& operator = (stack const& rhs)
{
stack tmp(rhs);
swap(tmp);
return *this;
}
FYI, the copy-and-swap idiom can be written more concisely if you want to:
stack& operator=(stack const& rhs)
{
stack(rhs).swap(*this);
return *this;
}
const void pop() {
The const here is useless; remove it. (I wonder if it was a mistake for something similar — void pop() const? constexpr void pop()? but neither of those makes sense either.)
T peek() {
OTOH, this method should be const:
T peek() const {
if (head == nullptr) throw std::underflow_error("cannot get item from empty stack");
return std::as_const(head->data);
}
I threw in an as_const just to illustrate complete paranoia. We don't know what type T is, right? So when you construct the return value T, which constructor do you want to call — T(T&) or T(const T&)? Write a unit test for a type T where it makes a difference, and see what happens.
getSize(), isFull(), and isEmpty() should all be const methods as well.
void swap(stack& other) noexcept
Good! You also do the using std::swap; dance correctly — although, since you're not swapping anything but ints and pointers, the dance is unnecessary in this case, but hey it's good practice.
You did forget, though, that if you want the using std::swap; dance to work for anyone else who uses your stack class, you'll need to provide that free ADL function swap. So we add an inline friend:
friend void swap(stack& a, stack& b) noexcept {
a.swap(b);
}
Now
using std::swap;
swap(redstack, bluestack);
will be as efficient as possible.
Consider adding move semantics to your class — stack(stack&&) noexcept, stack& operator=(stack&&) noexcept, void push(T&&) (or just void push(T) at that point). I assume you left them out on purpose for this simple example.
Your copyList is recursive. This could blow your stack if you're copying a very long list. You successfully eliminated the recursion from your destructor — why not eliminate it here too?
node* copyList(node* l)
Since this member function doesn't need the this pointer for anything, it should be marked static. And it wouldn't hurt to add const to the node you don't intend to modify, just for a tiny bit of self-documentation:
static node *copyList(const node *l)