首先,一些在这种情况下不会导致问题的通用 cmets,但在其他情况下可能会出现问题:
您应该只抛出从类std::exception 派生的异常。 C++ 确实允许你抛出任何类型(比如字符串,在你的情况下),但这是一个非常糟糕的主意。
类成员应该使用初始化列表进行初始化,如下所示:(这在其他情况下可能会导致错误。如果不使用初始化列表,则首先默认构造成员,然后使用赋值运算符在构造函数的主体中覆盖它们。并非所有类型都有赋值运算符,或者赋值运算符可能有不良副作用,因此不使用初始化列表可能会有问题)
Node(int val, Node* ptrToLast) : value(val), prev(ptrToLast) {}
Stack() : size(0), top(NULL) {}
给你的函数命名return*我们毫无意义。就叫他们size()和topPtr(),或者getSize()和getTopPtr()
第二,你没有遵守规则。 ;)
您的堆栈类有两个成员变量,它只允许有一个。 :)
最后,破坏堆栈的事情:
当您尝试取消引用空指针时,这将崩溃:
void test() {
Stack s;
s.peek(); // crashes
}
这会泄漏内存,因为分配的节点永远不会被删除(堆栈析构函数应该这样做):
void test() {
Stack s;
s.push(1);
}
析构函数应该如下所示:
~Stack() {
while (top != NULL){
Node* next = top.returnPtr();
delete top;
top = next;
}
}
这个也应该很有趣:
void test() {
Stack s;
s.push(1);
Stack t(s);
s.pop();
}
t.returnSize() 现在将返回 1,但 t.top 指向刚刚删除的 s 中的节点。这应该通过为堆栈定义一个复制构造函数和一个赋值运算符来解决(也许也为节点类定义)
复制构造函数如下所示:
Stack(const Stack& s);
如果你初始化一个堆栈从另一个堆栈调用,就像上面一样。
赋值运算符如下所示:
Stack& operator= (const Stack& s);
如果我将一个堆栈分配给另一个堆栈,则在两者都初始化后调用:
Stack s;
Stack t;
t = s; // now both are already initialized, so the assigment operator is used, not the copy constructor
这些函数的作用是确保 t 成为s 的副本。所以s中的每个节点都应该被复制,并分配给t,以避免它们指向相同的节点。 (顺便说一句,这是您之前关于所有权问题的一个很好的例子。节点应该完全属于一个 Stack 对象。如果它在多个对象之间共享,那么您就有问题了,它变成崩溃只是时间问题)
最后,如果我们想变得更糟:
void test() {
Stack s;
s.push(1);
s.push(2);
}
如果第二个节点的内存分配失败会发生什么(也许我们的内存用完了。不太可能,但它可能会发生)。 在您增加大小后会引发异常。现在 s 的大小为 2,即使 top 仍然指向第一个节点
如果你认为这个问题不太可能被认真对待,想象一下你的班级的一个小扩展。假设它是一个模板,因此它可以存储除 int 之外的其他类型。
这意味着每次创建节点时,我们都必须调用值类型的复制构造函数。这也可能引发异常。我们不知道,因为我们不知道用户可能会尝试将哪种类型存储在堆栈中。
“异常安全”的概念很重要,而且真的很难做到正确。基本上,如果抛出异常,您的班级处于哪个状态?它是否仍处于有效状态? (应该总是这样)。它是否丢失了任何数据(对于某些可能无法避免的情况,对于其他情况可以小心避免),如果它丢失了数据,是否已正确删除?调用析构函数,释放内存? (再次重申,应该始终如此)
最后一点是为什么我如此确定你至少会有一个错误。每个人都弄错了异常安全,包括我在内。在 C++ 中编写一个正确 实现像堆栈这样简单的东西是非常困难的。 :)
奖励:
为了回应 cmets 关于复制构造函数、析构函数和 RAII 的问题,让我们完成整个事情:
首先,让我说可能还有一两个我没有发现的错误。
其次,这是我测试过的代码,以下所有代码都通过了。也可以随意通过它运行您自己的代码。 (它应该按原样工作,除非您必须重命名 getSize 函数):(live 变量是我为调试添加的变量。我已经修改了我的 Stack 实现,以便构造函数递增它,而析构函数递减它,只是为了验证构造和破坏的数量是否相等。一旦你确定它有效,显然应该从 Stack 类中删除它
测试代码
static int live; // debugging - keeps track of how many nodes have been allocated. Constructors add 1, destructors subtract. It should end in 0
#include "stack.h"
#include <iostream>
#include <cassert>
int main(){
{
// test stack creation + push
Stack s;
s.push(1);
s.push(2);
s.push(3);
assert(s.getSize() == 3);
Stack t;
t.push(4);
t.push(5);
t.push(6);
assert(t.getSize() == 3);
// test assigment operator when both stacks contain data
s = t;
assert(s.getSize() == 3);
assert(s.peek() == 6);
assert(t.peek() == 6);
Stack u(s);
// test self assigment when stack contains data
u = u;
assert(u.getSize() == 3);
assert(u.peek() == 6);
Stack v;
// test copy construction from stack with data
Stack w(t);
assert(w.getSize() == 3);
assert(w.peek() == 6);
assert(t.getSize() == 3);
assert(t.peek() == 6);
// test assignment operator when source is empty, destination contains data
w = v;
assert(w.getSize() == 0);
assert(v.getSize() == 0);
// test copy construction from empty stack
Stack x(v);
assert(x.getSize() == 0);
assert(v.getSize() == 0);
// test pop
assert(t.pop() == 6);
assert(t.pop() == 5);
assert(t.pop() == 4);
assert(s.pop() == 6);
assert(s.pop() == 5);
assert(s.pop() == 4);
} // at this point, all allocated stacks go out of scope, so their destructors are called, so now is a good time to check for memory leaks:
assert(live == 0);
}
固定实现
现在,首先是简单的修复。 Stack 类中添加了复制构造函数、赋值运算符和析构函数。如果单独使用 Node 类仍然存在问题,但只要它仅通过 Stack 使用,我们就可以确保节点被正确复制和删除。不幸的是,Stack 现在需要访问Node.tail_ 才能进行复制,所以我将其交为朋友。所以它可以工作,但它并不优雅。
#include <stdexcept> // for std::exception
class Stack;
class Node
{
private: // changed naming to head/tail, which are commonly used in implementations of linked lists like this. The head is the current element, tail is a pointer to the remainder
int head_;
Node* tail_;
public:
friend class Stack; // this is necessary for the Stack copy constructor in order to modify the tail pointer after the node is created.
// the elegant solution had been to define a copy constructor on the Node class as well, but we'll get to that
int head() const { return head_; }
Node* tail() const { return tail_; }
Node(int val, Node* prev) : head_(val), tail_(prev) { ++live; } // use initializer list
~Node() { --live; }
Node(const Node& other) : head_(other.head_), tail_(other.tail_){ ++live; }; // this could be omitted, but I use it to update 'live' for debugging purposes
};
class Stack
{
private:
Node* top;
// int size; // we don't actually need the size at all, according to spec, so I removed it to keep things simple
bool empty() const { return top == NULL;}
void freeNodes() { // helper function to avoid duplicate code
while (!empty()){
pop();
}
}
public:
Stack() : top() {} // use initializer list
~Stack() { // destructor - the stack is being deleted, make sure to clean up all nodes
freeNodes();
}
Stack(const Stack& other) : top() { // copy constuctor - we're being initialized as a copy of another stack, so make a copy of its contents
if (other.empty()){
return;
}
top = new Node(*other.top); // copy the first node, to get us started
Node* otherNext = other.top->tail();
Node* current = top;
while (otherNext != NULL){
current->tail_ = new Node(*otherNext); // copy the current node
current = current->tail(); // move to the next node
otherNext = otherNext->tail();
}
}
Stack& operator= (const Stack& other) {
if (this == &other){ // If we assign this stack to itself (s = s), bail out early before we screw anything up
return *this;
}
//now create the copy
try {
if (other.empty()){
freeNodes();
top = NULL;
return *this;
}
// naively, we'd first free our own stack's data before constructing the copy
// but what happens then if an exception is thrown while creating the copy? We've lost all the current data, so we can't even roll back to a previous state
// so instead, let's simply construct the copy elsewhere
// this is almost straight copy/paste from the copy constructor. Should be factored out into a helper function to avoid duplicate code
Node* newTop = new Node(*other.top); // copy the first node, to get us started
Node* otherNext = other.top->tail();
Node* current = newTop;
while (otherNext != NULL){
current->tail_ = new Node(*otherNext); // copy the current node
current = current->tail(); // move to the next node
otherNext = otherNext->tail();
}
// once we're sure that we're able to create the copy of the other stack, we're ready to free the current one
// this is a bit of duplicate code
freeNodes();
top = newTop;
return *this;
}
catch (...){ // if an exception was thrown
throw; // and rethrow the exception so the application can deal with it
}
}
// Node* returnTopPtr() { return top; } // not necessary. It's not a required part of the public interface, and class members can just access the top variable directly
void push(int);
int pop();
int peek() const;
int getSize() const{
if (empty()){ return 0; }
int i = 0;
for (Node* cur = top; cur != NULL; cur = cur->tail_, ++i){}
return i;
}
};
void Stack::push(int value)
{
Node* currentTop = top;
top = new Node(value, currentTop); // this could throw an exception, but if it does, our stack will simply be left unchanged, so that's ok
}
int Stack::peek() const
{
if (empty()){
throw std::exception("Stack is empty");
}
return top->head();
}
int Stack::pop()
{
if (empty()){
throw std::exception("Stack is empty");
}
Node* tail = top->tail();
int result = top->head();
delete top;
top = tail;
return result;
}
RAII v. 1
RAII 是一项重要技术的糟糕名称。基本思想是每个资源分配(包括但不限于new. 的内存分配)都应该包装在一个类中,该类负责根据需要复制或删除资源。
在我们的例子中,不是让Stack 跟踪所有节点,我们可以通过让Node 类本身完成大部分工作来简化一些事情。现在Node 也被赋予了复制构造函数、赋值运算符和析构函数。堆栈现在只需要跟踪top 节点......几乎。它仍然有点不确定,因为Stack.push 分配了新节点,但Node 现在负责大部分删除。 .但是,它确实让我们摆脱了删除或复制节点列表之前需要的循环。
Stack still needs to access thetail_member ofNode`,但是这一次,我做了一个访问函数而不是让类成为成员。总的来说,更好,但我仍然不满意。
#include <stdexcept>
class Node
{
private:
int head_;
Node* tail_;
public:
int head() const { return head_; }
Node* tail() const { return tail_; }
Node*& tail() { return tail_; } // Another way to allow Stack to modify the tail. Needed for pop()
Node(int val, Node* prev = NULL) : head_(val), tail_(prev) { ++live; }
~Node(){ --live; delete tail_; } // it is safe to call delete on a NULL pointer
Node(const Node& other) : head_(other.head()), tail_(NULL) {
++live;
if (other.tail() == NULL){
return;
}
tail_ = new Node(*other.tail());
}
Node& operator= (const Node& other){
if (this == &other){
return *this;
}
head_ = other.head();
if (other.tail() != NULL){
return *this;
}
Node* oldTail = tail_;
try {
tail_ = new Node(*other.tail());
}
catch(...){
tail_ = oldTail;
throw;
}
}
};
class Stack
{
private:
Node* top;
bool empty() const { return top == NULL;}
public:
Stack() : top() {}
~Stack() {
delete top;
}
Stack(const Stack& other) : top(){
if (other.empty()){
return;
}
top = new Node(*other.top);
}
Stack& operator= (const Stack& other) {
if (this == &other){
return *this;
}
Node* oldTop = top;
try {
top = NULL;
if (other.top != NULL){
top = new Node(*other.top);
}
delete oldTop;
return *this;
}
catch (...){
delete top;
top = oldTop;
throw;
}
}
void push(int);
int pop();
int peek() const;
int getSize() const{
if (empty()){ return 0; }
int i = 0;
for (Node* cur = top; cur != NULL; cur = cur->tail(), ++i){}
return i;
}
};
void Stack::push(int value)
{
Node* currentTop = top;
top = new Node(value, currentTop);
}
int Stack::peek() const
{
if (empty()){
throw std::exception("Stack is empty");
}
return top->head();
}
int Stack::pop()
{
if (empty()){
throw std::exception("Stack is empty");
}
Node* tail = top->tail();
int result = top->head();
if (top != NULL){
top->tail() = NULL; // detach top from the rest of the list
delete top;
}
top = tail;
return result;
}
RAII v.2
为了解决上面提到的问题,我决定稍微改变一下我的策略。 Node 现在完成所有繁重的工作,包括推送/弹出/peek 操作。 Stack 只是这些的薄包装。事实证明,这解决了大部分问题。 Stack 不再需要与 Node 的私人成员混在一起,我们对所有权有了一些更明确的规则。堆栈拥有顶部节点,每个非顶部节点都归其父节点所有——这一次,所有者 both 创建、复制和销毁该节点。更加一致。
为了实现这一点,我必须在 Node 类上添加一个 isLast 函数,否则,Stack.pop 无法知道是否该删除 top。我对这个解决方案也不是 100% 满意(如果我没有从堆栈中删除 size 成员,我可以用它来解决问题)
但总的来说,这个比上述尝试更干净、更简单。 (这是我唯一花了不到一个小时调试的,一方面。;))
#include <stdexcept>
class Node {
public:
Node(int value, Node* prev = 0) : head(value), tail(prev) { ++live;}
~Node() {
--live;
delete tail;
}
Node(const Node& other) : head(other.head), tail(0) {
++live;
if (other.tail != 0){
tail = new Node(*other.tail);
}
}
Node& operator= (const Node& other){
if (this == &other){
return *this;
}
Node* oldTail = tail;
tail = new Node(*other.tail);
delete oldTail;
head = other.head;
return *this;
}
void push(int val){
tail = new Node(head, tail);
head = val;
}
int peek(){
return head;
}
void pop(){
Node* oldTail = tail;
head = tail->head;
tail = tail->tail; // lol
oldTail->tail = 0;
delete oldTail;
}
bool isLast() { return tail == NULL; }
int getSize() const{
int i = 0;
for (const Node* cur = this; cur != NULL; cur = cur->tail, ++i){}
return i;
}
private:
Node* tail;
int head;
};
class Stack {
public:
Stack() : top(){}
~Stack() { delete top; }
Stack(const Stack& other) : top() {
if (other.empty()){
return;
}
top = new Node(*other.top);
}
Stack& operator= (const Stack& other){
if (this == &other){
return *this;
}
Node* newTop = NULL;
if (!other.empty()){
newTop = new Node(*other.top);
}
delete top;
top = newTop;
return *this;
}
void push(int val){
if (empty()) {
top = new Node(val);
}
else {
top->push(val);
}
}
int peek(){
if (empty()){
throw std::exception("Empty stack");
}
return top->peek();
}
int pop(){
int result = peek();
if (top->isLast()){
delete top;
top = NULL;
}
else {
top->pop();
}
return result;
}
int getSize() const{
if (empty()){ return 0; }
return top->getSize();
}
private:
bool empty() const { return top == NULL; }
Node* top;
};
既然所有这一切都是为了向您展示为什么 C++ 不是一个很好的初学者语言,我想我可以肯定地说任务完成了!
:)