C++ 指针/列表实现

Question

编写一个具有以下属性的类 ListNode：

• int 值；
• ListNode *next;

提供以下功能：

• ListNode(int v, ListNode *l)
• int getValue();
• ListNode* getNext();
• 无效插入（int i）；
• bool listcontains(int j);

编写一个程序，要求用户输入一些整数并将它们存储为 ListNodes，然后询问它应该在列表中查找的数字。

这是我的代码：

#include <iostream>
using namespace std;

class ListNode
{
    private:
        struct Node
        {
            int value;
            Node *next;
        } *lnFirst;

    public:
        ListNode();
        int Length();       
        void DisplayList();     
        void Insert( int num );
        bool Contains( int num );       
        int GetValue( int num );        
};

ListNode::ListNode()
{
    lnFirst = NULL;
}

int ListNode::Length()
{
    Node *lnTemp;
    int intCount = 0;
    for( lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next )
    {
        intCount++;
    }
    return intCount;
}

void ListNode::DisplayList()
{
    Node *lnTemp;
    for( lnTemp = lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next )
        cout<<endl<<lnTemp->value;
}

void ListNode::Insert(int num)
{
    Node *lnCurrent, *lnNew;

    if( lnFirst == NULL )
    {
        lnFirst = new Node;
        lnFirst->value = num;
        lnFirst->next = NULL;
    }
    else
    {
        lnCurrent = lnFirst;
        while( lnCurrent->next != NULL )
            lnCurrent = lnCurrent->next;

        lnNew = new Node;
        lnNew->value = num;
        lnNew->next = NULL;
        lnCurrent->next = lnNew;
    }
}

bool ListNode::Contains(int num)
{
    bool boolDoesContain = false;
    Node *lnTemp,*lnCurrent;
    lnCurrent = lnFirst;
    lnTemp = lnCurrent;
    while( lnCurrent!=NULL )
    {
        if( lnCurrent->value == num )
        {
            boolDoesContain = true;
            return boolDoesContain;
        }
        lnTemp = lnCurrent;
        lnCurrent = lnCurrent->next;
    }
    return boolDoesContain;
}

int ListNode::GetValue(int num)
{
    Node *lnTemp;
    int intCount = 1;
    for( lnTemp=lnFirst; lnTemp != NULL; lnTemp = lnTemp->next )
    {
        if (intCount == num)
        {
            return lnTemp->value;
        }
        intCount++;
    }    
}

int main()
{   
    cout << "Input integers below. Input the integer -1 to stop inputting.\n\n";
    
    ListNode lnList;
    int intNode = 1, intInput = 0;
    while (intInput != -1) {
        cout << "Please input integer number " << intNode << ": "; cin >> intInput;
        intNode++;
        if (intInput != -1) { lnList.Insert(intInput); }
    }   
    
    lnList.DisplayList();
    cout << "\n\n";
    
    int intListLength = lnList.Length();    
    cout << "Which value do you wish to recall? (Between 1 and " << intListLength << "): "; cin >> intNode;    
    if ( intNode >= 1 && intNode <= intListLength ) {
        cout << "Value at position " << intNode << " is " << lnList.GetValue(intNode) << ".";                
    } else {
        cout << "No such position in the list. Positions run from 1 to " << intListLength << ". You asked for " << intNode << ".";
    }
    
    cout << "\n\nCheck if the following value is in the list: "; cin >> intNode;
    bool IsThere = lnList.Contains(intNode);
    if (IsThere) {
        cout << intNode << " is in the list.";
    } else {
        cout << intNode << " is not in the list.";
    }
    
    cout << "\n\n";
    system("pause");
    return 0;  
}

我们可以在哪里改进？

Answer 1

unwind 和 ckarmann 怎么说。这是一个提示，我为您实现了 listcontains，让您了解分配的含义：

class ListNode {
private:
    int value;
    ListNode * next;

public:
    bool listcontains(int v) { 
        // does this node contain the value?
        if(value == v) return true; 

        // was this the last node?
        if(next == 0) return false;

        // return whether nodes after us contain the value 
        return next->listcontains(v);
    }
};

因此，您只有列表的头部，它依次链接到下一个节点。尾部会有next == 0;

Answer 2

我认为您误解了所要求的设计。 ListNode 类应该是一个节点，而不是包含节点的列表。

我建议您不要将多个命令放在一个 ligne 上，如下所示：

cout << "Please input integer number " << intNode << ": "; cin >> intInput;

这简直令人困惑。

Answer 3

在更一般的样式注释中，将指针声明为更接近您定义它们的位置，并使其范围尽可能小。
虽然您的代码在技术上不会出错，但根据我的经验，始终这样做可以避免更大/更旧的代码库中的错误。

例如而不是

Node *lnTemp;
int intCount = 0;
for( lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next )
{
}

写

int intCount = 0;
for(Node* lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next )
{
}

或者，类似的，而不是

Node *lnTemp,*lnCurrent;
lnCurrent = lnFirst;
lnTemp = lnCurrent;

写

Node* lnCurrent = lnFirst;
Node* lnTemp = lnCurrent;

Answer 4

我认为您过度设计了列表节点建模。 ListNode 类 IS A 列表节点，从它的名字就可以看出。那么它应该不需要嵌套结构来保存列表信息，这很令人困惑。

Answer 5

本文底部有更详细的反馈，但首先，只是一些内联 cmets 和代码更改：

struct Node // Why doesn't this have a constructor initializing the members?
{
        int value;
        Node *next;
} *lnFirst; 


ListNode::ListNode() : lnFirst(NULL) {} // Use initializer lists instead of initializing members in the ctor body. It's cleaner, more efficient and may avoid some nasty bugs (because otherwise the member gets default-initialized *first*, and then assigned to in the body)

int ListNode::Length()
{
    int intCount = 0;
    for( Node* lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // Create the loop iteration variable lnTemp here, in the loop, not at the start of the function
    {
        intCount++;
    }
    return intCount;
}

void ListNode::DisplayList()
{
    for(Node* lnTemp = lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // And again, initialize the loop variable in the loop
        cout<<endl<<lnTemp->value; // Not a huge deal, but endl flushes the stream as well as inserting a newline. That can be needlessly slow. So you might want to just use "\n" in cases where you don't need the flushing behavior.
}

void ListNode::Insert(int num)
{
//    Node *lnCurrent, *lnNew; // Very subjective, but I prefer not declaring multiple variables on the same line, because the syntax if they're pointers can be surprising (You got it right, but a lot of people would write Node* lnCurrent, lnView, which would make lnView not a pointer. I find it clearer to just give ecah variable a separate line:
    if( lnFirst == NULL )
    {
//        lnFirst = new Node;
//        lnFirst->value = num;
//        lnFirst->next = NULL;
        lnFirst = new Node(num); // Make a constructor which initializes next to NULL, and sets value = num. Just like you would in other languages. ;)
    }
    else
    {
        Node* lnCurrent = lnFirst; // Don't declare variables until you need them. Both to improve readability, and because C++ distinguishes between initialization and assignment, so in some cases, default-initialization followed by assigment may not be the same as just initializing with the desired value.
        while( lnCurrent->next != NULL )
                lnCurrent = lnCurrent->next;

        Node* lnNew = new Node(num); // Again, let a constructor do the work.
        lnCurrent->next = lnNew;
    }
}

bool ListNode::Contains(int num)
{
    bool boolDoesContain = false;
//    Node *lnTemp,*lnCurrent; // Again, don't initialize variables at the start of the function if they're not needed
    Node* lnCurrent = lnFirst;
//    lnTemp = lnCurrent;
    while( lnCurrent!=NULL )
    {
        if( lnCurrent->value == num )
        {
//                boolDoesContain = true;
//                return boolDoesContain;
                  return true; // Just return directly, and remove the boolDoesContain variable. Alternatively, set boolDoesContain to true, and then break out of the loop without returning, so you have a single exit point from the function. Both approaches have their merits, but setting a variable you don't need, and then returning is silly. ;)
        }
//        Node* lnTemp = lnCurrent; // you don't actually use lnTemp for anything, it seems
        lnCurrent = lnCurrent->next;
    }
//    return boolDoesContain;
      return false; // just return false. If you get this far, it must be because you haven't found a match, so boolDoesContain can only be false anyway.
}

int ListNode::GetValue(int num)
{
//    Node *lnTemp;
    int intCount = 1; // Wouldn't most people expect this indexing to be zero-based?
    for( Node* lnTemp=lnFirst; lnTemp != NULL; lnTemp = lnTemp->next )
    {
        if (intCount == num)
        {
            return lnTemp->value;
        }
        intCount++;
    }    
}

现在，有几个通用的 cmets。 （我将忽略您是否误解了作业，并专注于您发布的代码） 首先，匈牙利符号：不要。首先调用你的节点指针，临时和其他任何东西，没有'ln'前缀。调用你的布尔变量 doesContain 没有不必要的'bool'前缀。 其次，正如我在编辑后的代码中所做的那样，只在需要时创建变量。 C 曾经要求在块的顶部声明变量，但 C++ 从未这样做过。 第三，您不需要在 main 函数的末尾加上“return 0”。 Main 是一种特殊情况，如果它到达函数的末尾，它会自动返回 0。

第四，我们有一个令人讨厌的大问题：内存管理。 您分配的内存永远不会被释放。由于您没有 RemoveNode 函数，这似乎是一个有争议的问题，但是当整个列表本身超出范围并被删除时会发生什么？它的所有节点都没有被删除，因为列表中只有一堆指针，并且它不会自动在这些指针上调用 delete。 所以最起码，你需要一个列表类本身的析构函数，这样当列表被删除时，它会确保删除它的所有子节点。

这应该可以处理简单的默认情况，即创建一个列表、向其中添加节点并删除该列表。

下一个大问题，如果我复制列表呢？

int main(){
ListNode list;
list.Insert(1);
list.Insert(2);
list.Insert(3);
}
ListNode list2 = list;

您的代码爆炸了。两个列表现在都指向 same 节点，而不是制作节点的副本。将节点添加到一个列表也会使其显示在另一个列表中。 在您声称“这是一项功能，而不是错误”;) 之前，请考虑当其中一个列表被删除时会发生什么现在。

假设 list2 首先被删除。使用我们刚刚定义的析构函数，它删除了三个节点，然后返回。 现在列出已删除节点的点。访问它们是未定义的行为，并且很可能会崩溃。因此，假设我们不访问它们，而是快速删除此列表。

糟糕，这意味着我们正在尝试删除已被删除的子节点。这绝对听起来像是一场崩溃。

所以要解决这个问题，您的 ListNode 类必须实现两个附加函数，复制构造函数和赋值运算符：

ListNode(const ListNode& other);
ListNode& operator==(const ListNode& other);

这两个必须确保当所有数据复制来自'other'。对于“其他”中的每个节点，您必须在当前列表中分配一个新节点，而不是让两个列表都指向同一个节点。 （这意味着节点类很可能至少还需要一个复制构造函数）。

这是处理内存管理的基本方法，理解这一点很重要，否则你会搞砸。 ;)

Answer 6

部分作业如下：

提供以下功能：

• ListNode(int v, ListNode *l)
• int getValue();
• ListNode* getNext();
• 无效插入（int i）；
• bool listcontains(int j);

您没有提供任何这些功能。

正如其他几个人指出的那样，您实现了 List 而不是 ListNode，这就是您的函数签名不同的原因。

但您也不应该轻率地违反作业的编码约定。你有 C# 背景吗？在 C++ 中，编码约定通常要求使用小写的方法名称。

Answer 7

另一个改进，在列表代码中，您不应该遍历整个列表来获取长度，您可以保留一个计数器，了解在插入/删除时更新它的元素数量并返回它。