1

こんにちは。動的メモリ割り当て、ポインター、クラス、および例外を含むリンク リストの演習を行っています。誰かがそれを批判して、スタイルと上に挙げたテーマの両方に関して、私がどこを間違えたのか、何を改善すべきだったのかを教えてくれませんか?

/*

Linked List exercise

*/

#include <iostream>
#include <exception>
#include <string>
using namespace std;

class node{
public:
    node * next;
    int * data;
    node(const int i){
        data = new int;
        *data = i;
    }
    node& operator=(node n){
        *data = *(n.data);
    }
    ~node(){
        delete data;
    }
};

class linkedList{

public:

    node * head;
    node * tail;
    int nodeCount;

    linkedList(){
        head = NULL;
        tail = NULL;
    }

    ~linkedList(){
        while (head){
            node* t = head->next;
            delete head;
            if (t) head = t;
        }
    }

    void add(node * n){
        if (!head) {
            head = n;
            head->next = NULL;
            tail = head;
            nodeCount = 0;
        }else {
            node * t = head;
            while (t->next) t = t->next;
            t->next = n;
            n->next = NULL;
            nodeCount++;
        }
    }

    node * operator[](const int &i){
        if ((i >= 0) && (i < nodeCount)) throw new exception("ERROR:  Invalid index on linked list.", -1);
        node *t = head;
        for (int x = i; x < nodeCount; x++) t = t->next;
        return t;
    }

    void print(){
        if (!head) return;
        node * t = head;
        string collection;
        cout << "[";
        int c = 0;
        if (!t->next) cout << *(t->data);
        else while (t->next){
            cout << *(t->data);
            c++;
            if (t->next) t = t->next;
            if (c < nodeCount) cout << ", ";
        }
        cout << "]" << endl;
    }

};

int main (const int & argc, const char * argv[]){

    try{
        linkedList * myList = new linkedList;
        for (int x = 0; x < 10; x++) myList->add(new node(x));
        myList->print();
    }catch(exception &ex){
        cout << ex.what() << endl;
        return -1;
    }   
    return 0;
}
4

4 に答える 4

7

データがポインターである必要はありません。

ctor-initializer リストを使用してください。これは、const の正確性と例外の安全性に優れています。コンストラクターのいずれも、本体内にコードを含める必要はありません。

linkedList コンストラクターが nodeCount を初期化していません。

リストのテールポインターを何にも使用していません。add が空でない場合に、リスト全体をスキャンする手間を省くことができます (最新の状態に保っていれば)。

索引付け (operator[]) は、リンクされたリストでサポートするのは珍しいことです。OTOHは削除機能を作成していません。

operator[] は、そのパラメーターを参照によって受け取るべきではありません。大きな構造体のみを const 参照で渡す必要があり、int のような小さな型は値で渡す必要があります。

現在、add が失敗すると、new node() へのポインターがリークします。(しかし、リストのリンクが壊れていない限り、add が失敗する方法は実際にはわかりません。)

データの二重解放を防ぐために、ノードでプライベート コピー コンストラクターを定義する必要があります。operator= とデストラクタを定義するときはいつでも、コピー コンストラクタも定義または削除する必要があります (3 つのルール)。また、二重解放を防ぐために、linkedList でプライベート コピー コンストラクターと代入演算子を定義する必要があります。

print 関数の可変文字列コレクションが使用されていません。print() の変数 t は const へのポインタでなければなりません。print 自体は const メンバー関数である必要があります。

于 2010-03-13T19:18:21.927 に答える
4 
/*
Linked List exercise
*/

class node {
public:
    node * next;
    int * data;

dataポインターである必要はありません (教室での課題でポインターである必要があると言われていない限り)。これを に変更してintから、参照を から*(t->data)に変更できますt->data。同様に、 ctor/dtor でnewandを使用する必要はありません。delete

    node(const int i) {
        data = new int;
        *data = i;
    } 
    node& operator=(node n) {
        *data = *(n.data);
    } 
    ~node() {
        delete data;
    } 
};

class linkedList {

public:
    node * head;
    node * tail;
    int nodeCount;

    linkedList() {
        head = NULL;
        tail = NULL;

ここですべてのデータ メンバーを初期化する必要があります。nodeCountここでゼロに設定しない限り、ランダムな値になります。

    } 

    ~linkedList() {
        while (head) {
            node* t = head->next;
            delete head;
            if (t)
                head = t;

これはバグのようです。t が であっても、常に を割り当てる必要があります。そうしないと、ループが終了しません (ただし、同じノードを複数回削除すると、おそらく例外が発生します)。head = tNULL

        } 
    } 

    void add(node * n) {
        if (!head) {
            head = n;
            head->next = NULL;
            tail = head;
            nodeCount = 0;

ifこの発言全体は不要だと思います。リンクされたリストが空の場合、両方のブランチは同じことを行うため、常に 2 番目の ( else ) ブランチを使用できます。

また、最後に を設定nodeCountする0のはバグのようです。そうあるべきではない1ですか？

        } else {
            node * t = head;

            while (t->next)
                t = t->next;

            t->next = n;
            n->next = NULL;
            nodeCount++;
        } 
    } 

    node * operator[](const int &i) {

些細な問題ですが、const int &議論はまったく意味がありません。型を渡すだけでは何も得られませんint。

        if ((i >= 0) && (i < nodeCount))
            throw new exception("ERROR:  Invalid index on linked list.", -1);

上記の条件は私には逆に見えます。常に有効なパラメーターで例外をスローします。

        node *t = head;

        for (int x = i; x < nodeCount; x++)
            t = t->next;

上記のループは、私には少し疑わしいようです。iが必要なノードのゼロベースのインデックスである場合、カウンターは から に移動すべきではありません0かi-1?

        return t;
    } 

    void print() {
        if (!head)
            return;

繰り返しますが、このメソッドは、空のリストを防ぐ必要なく機能するようにする必要があります。空のリストは次のよう[]に出力されると思いますが、上記のガード コードを使用すると、何も出力されません。

        node * t = head;
        string collection;
        int c = 0;

        cout << "[";

        if (!t->next) {
            cout << *(t->data);

上記の他の条件と同様に、上記のブランチは不要だと思います。2 番目の ( else ) ブランチは、NULLケースを適切に処理する必要があります。

        } else {
            while (t->next) {

私はあなたがwhile (t)上を望んでいないと思いますwhile (t->next)

                cout << *(t->data);
                c++;

                if (t->next)
                    t = t->next;

これは先ほどのバグと同じだと思います。常に`t = t->next' を割り当てる必要があります。そうしないと、ループが終了しません。

                if (c < nodeCount)
                    cout << ", ";
            } 
        } 

        cout << "]" << endl;
    } 
};

int main (const int & argc, const char * argv[]) { // const int& ?

const int &また...

    try {
        linkedList * myList = new linkedList;
        for (int x = 0; x < 10; x++)
            myList->add(new node(x));
        myList->print();
    } catch(exception &ex) {
        cout << ex.what() << endl;
        return -1;
    }

宿題にはおそらく問題ありませんが、(上記で行っているように) 考えられるすべての例外をキャッチすることは、おそらくここではあまり価値がなく、一般的に悪い習慣と見なされます。

上記の try/catch を省略した場合、プログラムが例外で終了するたびに、コンパイラによって提供されるデフォルトのトップレベルの例外ハンドラーが例外をダンプし、トレース情報をスタックするのではないかと思います (コンパイラによって異なります)。

    return 0;
}
于 2010-03-13T20:02:26.930 に答える
0 
if ((i >= 0) && (i < nodeCount)) throw new exception("ERROR:  Invalid index on linked list.", -1);

その行は次のようになります。

if( i < 0 || i >= nodeCount ) throw...

add メソッドにも問題があります。

void add(node * n){
    if (!head) {
        head = n;
        head->next = NULL;
        tail = head;
        nodeCount = 0;
    }else {
        node * t = head;
        while (t->next) t = t->next;
        t->next = n;
        n->next = NULL;
        nodeCount++;
    }
}

最初のノード数を追加した後の 1 つは、リンクされたリストに挿入するために検索を行っている別の場合です...人々が O(1) であると予想する操作です。

より典型的な実装は次のようになります。

void add( node* n ) {
   node* old_head = head;
   head = n;
   head->next = old_head;
   ++nodeCount;
   if( old_head == NULL )
      tail = head
}
于 2010-03-13T19:19:59.087 に答える
0

1)data = new int(i)フィールドの初期化に使用できます。

2) operator[]if ((i >= 0) && (i < nodeCount))で間違った条件を作成しました。次のように反転する必要があります。if ((i < 0) || (i >= nodeCount))

于 2010-03-13T19:20:47.667 に答える