I hope the commentary on each piece of the answer demonstrates the principles well enough you can learn more than just these specific solutions.
Though you don't show the definitions of the class templates, it looks like LinkedStack<ItemType>
has a private member topPtr
, and you'd like to change its type from Node<ItemType>*
to std::unique_ptr<Node<ItemType>>
. Also, class template Node
has member functions something like:
template <class ItemType>
class Node
{
// ...
// Either public, or Node befriends LinkedStack in some way.
explicit Node(const ItemType& value, Node* next = nullptr);
Node* getNext() const { return nextPtr; }
void setNext(Node* ptr) { nextPtr = ptr; }
// ...
// Probably private:
Node* nextPtr;
};
Changes to this Node
template could look like:
template <class ItemType>
class Node
{
// ...
// Either public, or Node befriends LinkedStack in some way.
explicit Node(const ItemType& value)
Node(const ItemType& value, std::unique_ptr<Node> next)
: Node(value) { nextPtr = std::move(next); }
Node* getNext() const { return nextPtr.get(); }
void setNext(std::unique_ptr<Node> ptr) { nextPtr = std::move(ptr); }
void resetNext() { nextPtr = nullptr; }
[[nodiscard]] std::unique_ptr<Node> releaseNext()
{ return std::move(nextPtr); }
// ...
// Probably private:
std::unique_ptr<Node> nextPtr;
};
The member becomes a smart pointer because the Node
"owns" the responsibility for cleaning up its "next" node, if any.
The pointer parameters of the two-argument constructor and setNext
become smart pointers because calling each means the Node
will take over responsibility for cleaning up that next Node
. They need to use std::move
to allow the member to actually take that responsibility from the parameter.
You might ask, shouldn't getNext
return a smart pointer? No, because that would mean that calling getNext
transfers responsibility for cleaning up the next node to the caller, and that's usually not what getNext
is for. A raw pointer can still have its place, meaning a pointer to an object whose ownership is handled by something else, or possibly a null pointer. (When null isn't a possibility, we'd often consider changing from a raw pointer to a reference, which also implies ownership is handled by something else.)
Though for cases where we do want to take ownership back from a Node
, I've added releaseNext()
. It returns a unique_ptr
to "give away" its responsibility, and in the process its own unique_ptr
member becomes empty.
Finally, I've added resetNext
as a way to reset the nextPtr
back to null, more straightforward than passing an empty smart pointer to setNext
. (And/or, we could overload a setNext(std::nullptr_t)
.)
Then Node
can follow the Rule of Zero: It does not need to declare a destructor, copy constructor, move constructor, or any sort of assignment operator. Since it has a member of type std::unique_ptr<Node>
, its implicitly-declared copy constructor and copy assignment members will be defined as deleted, meaning the compiler will complain about any code that tries to use them. But it will have working move constructor and move assignment operator, which are probably not needed but harmless.
Now to class template LinkedStack
. It can't use the Rule of Zero since copying it should be allowed despite the unique_ptr
member and should do a deep copy. So we'll go with the Rule of Five, as modified by the Copy and Swap Idiom:
template <class ItemType>
class LinkedStack
{
public:
LinkedStack() = default;
LinkedStack(const LinkedStack&);
LinkedStack(LinkedStack&&) = default;
LinkedStack& operator=(LinkedStack) noexcept;
~LinkedStack() = default;
friend void swap(LinkedStack& s1, LinkedStack& s2) {
using std::swap;
swap(s1.topPtr, s2.topPtr);
}
// ...
private:
std::unique_ptr<Node<ItemType>> topPtr;
};
The destructor is okay to default, so with the = default;
above, you can delete your custom definition.
Assignment is defined per Copy And Swap:
template <class ItemType>
LinkedStack<ItemType>& LinkedStack<ItemType>::operator=(
LinkedStack rhs) noexcept
{
swap(*this, rhs);
return *this;
}
The copy constructor gets simpler:
template <typename ItemType>
LinkedStack<ItemType>::LinkedStack(const LinkedStack<ItemType>& aStack)
: topPtr() // initially null
{
if (aStack.topPtr) {
Node* origStackPtr = aStack.get();
topPtr = std::make_unique<ItemType>(origStackPtr->getItem());
Node* newStackPtr(topPtr.get());
origStackPtr = origStackPtr->getNext();
while (origStackPtr) {
newStackPtr->setNext(
std::make_unique<ItemType>(origStackPtr->getItem())
);
newStackPtr = newStackPtr->getNext();
origStackPtr = origStackPtr->getNext();
}
}
}
The loop uses raw Node*
pointers because once the nodes are safely stored in the topPtr
or in another Node
, the constructor code no longer needs to worry about deleting them. But it does need to reassign the variables as the loop executes, so references won't do, and it also needs to detect when origStackPtr->getNext()
returns a null pointer.
Your original needed the try-catch-rethrow because in case of an exception in a constructor body, the destructor for that class is not called. But destructors of its members and base classes are called. So now if an exception happens in the copy constructor body, the destructor for the unique_ptr
member topPtr
executes, which will take care of deleting the top Node
. Destruction of that Node
's unique_ptr
member will likewise delete its next node if any, and so on recursively - all with zero lines of (your) code.
isEmpty
does not need any change: the expression !topPtr
also works with a unique_ptr
, meaning "is not null".
A straightforward update of push
just changes the new
to a make_unique
:
template <typename ItemType>
bool LinkedStack<ItemType>::push(const ItemType& newItem) {
try {
topPtr = std::make_unique<Node<ItemType>>(
newItem, topPtr);
}
catch (const std::bad_alloc&) {
return false;
}
return true;
}
However, I'd recommend getting rid of the try
/catch
and return value. A memory error is rare, so most code shouldn't be checking to see whether this push called one. Something that really does care ought to do a try
/catch
itself around whatever amount of code is most appropriate. This would reduce the body to just one statement.
And pop
gets simpler:
template <typename ItemType>
bool LinkedStack<ItemType>::pop() {
if (!isEmpty()) {
topPtr = topPtr->releaseNext();
return true;
}
return false;
}
Notice in the topPtr
reassignment statement, first releaseNext()
is used to take responsibility for the second Node
(if any) away from the top Node
. Then that responsibility is immediately given to the topPtr
variable by the assignment. This also means topPtr
will delete what it previously pointed at, the top node being popped off. Since that top node no longer has a next node, nothing else gets deleted, and the stack ends up in the correct state just like in the old version.
peek()
does not require any change. The topPtr->getItem()
expression will work via the unique_ptr<T>::operator->
function. (But I'd suggest changing the return types of Node<ItemType>::getItem()
and LinkedStack<ItemType>::peek()
to const ItemType&
, to avoid useless copies when ItemType
is a class type.)