1

リンクされたリストを 1 つ作成しました。すべて正常に動作します。

コードで潜在的に危険なことを行ったかどうかを知りたいだけです。私が懸念しているコード スニペットは、プッシュ、ポップ、およびクリーンアップです。コードの一部はユーザーとのやり取りのためだけのものなので、それほど重要ではありません (何をしているかがより明確になるように投稿しました)。リンクされたリスト アプリケーションのみ。

これは私の最初の試みであるため、あらゆる提案に感謝します。

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct product_data product_data_t;
struct product_data
{
    int product_code;
    char product_name[128];
    int product_cost;
    product_data_t *next;
};

static product_data_t *head = NULL;
static product_data_t *tail = NULL;
static product_data_t *new_product = NULL;

// Push a product on to the list.
void push(int code, char name[], int cost); 
// Pop (delete) a product from the list.
void pop(int code);
// Display all product in the list.
void display_list();
// Delete all memory allocated on the list
void clean_up();
// Display menu
void menu();

int main(void)
{
    menu();

    getchar();

    return 0;
}

void push(int code, char name[], int cost)
{
    // Allocate memory for the new product
    new_product = calloc(1, sizeof(product_data_t));
    if(!new_product)
    {
        fprintf(stderr, "Cannot allocated memory");
        exit(1);
    }

    /* Populate new products elements fields */
    new_product->product_code = code;
    strncpy(new_product->product_name, name, sizeof(new_product->product_name));
    new_product->product_cost = cost;
    new_product->next = NULL;

    // Set the head and tail of the linked list
    if(head == NULL)
    {
        // First and only product
        head = new_product;
    }
    else
    {
        tail->next = new_product;
    }

    tail = new_product;
}

// Find the product by code and delete
void pop(int code)
{
    product_data_t *product = head;
    product_data_t *temp = NULL;
    product_data_t *previous = head;
    int found = 0; // 0 - Not Found, 1 - Found

    if(!head)
    {
        puts("The list is empty");
        return;
    }

    while(product)
    {
        if(product->product_code == code)
        {
            found = 1; // Found
            // Check if this is in the first node - deleting from head
            if(head->product_code == code)
            {
                temp = head;
                head = head->next;
                free(temp);

                // Finished Deleting product
                return;
            }

            // Check if this is the end node - deleting from the tail
            if(tail->product_code == code)
            {
                temp = tail;
                tail = previous;
                free(temp);

                // Finished deleting product
                return;
            }

            // delete from list if not a head or tail
            temp = product;
            previous->next = product->next;
            free(temp);

            // Finished deleting product
            return;
        }
        // Get the address of the previous pointer.
        previous = product;
        product = product->next;  
    }

    if(!found)
    {
        printf("code [ %d ] was not found\n", code);
    }

    // Set all to null after finished with them
    product = NULL;
    temp = NULL;
    previous = NULL;
}

// Traverse the linked list
void display_list()
{
    // Start at the beginning
    product_data_t *product = head;

    while(product)
    {
        printf("===================================\n");
        printf("Product code: \t\t%d\n", product->product_code);
        printf("Product name: \t\t%s\n", product->product_name);
        printf("product cost (USD): \t%d\n", product->product_cost);
        printf("===================================\n\n");

        // Point to the next product
        product = product->next;
    }
    // Finished set to null
    product = NULL;
}

// Release all resources
void clean_up()
{    
    product_data_t *temp = NULL;

    while(head)
    {
        temp = head;
        head = head->next;
        free(temp);    
    }
    head = NULL;
    temp = NULL;

    // End program - goodbye
    exit(0);
}

void menu()
{
    int choice = 0, code = 0, cost = 0;
    char name[128] = {0};

    do
    {
        fflush(stdin); // Flush the input buffer

        puts("========= Welecome to linked list ===============");
        puts("[1] Add new product to the list");
        puts("[2] Delete a product from the list");
        puts("[3] Display all products");
        puts("[4] Exit and clean up");
        printf("Enter your choice: ");
        scanf("%d", &choice);

        switch(choice)
        {
        case 1:
            printf("Enter product code: ");
            scanf("%d", &code);
            printf("Enter cost: ");
            scanf("%d", &cost);
            printf("Enter name: ");
            scanf("%s", name);
            push(code, name, cost);
            break;

        case 2:
            printf("Enter product code: ");
            scanf("%d", &code);
            pop(code);
            break;

        case 3:
            display_list();
            break;

        case 4:
            clean_up();
            break;

        default:
            puts("Incorrect choice");
            break;
        }
    }while(choice != 4);
}
4

7 に答える 7

9

pop() から

            if(head->product_code == code)
            {
                    temp = head;
                    head = head->next;
                    free(temp);

                    // Finished Deleting product
                    return;
            }

アイテムが 1 つしかない場合、'head' と 'tail' は同じノードを指します。ただし、この 1 つのアイテムをポップすると、'head' は調整されますが、'tail' は解放されたノードを指したままになります。これにより、不良ポインターが残り、コンピューターが爆発する可能性があります。

補遺: 同様に、プッシュされた最後のノードをポップすると、「new_product」がぶら下がり、clean_up() は「テール」ポインターもぶら下がったままになります。提供されているコード サンプルが解放された後にこれらを逆参照しない場合でも、C コードのダングリング ポインターは常に "潜在的に危険" として扱われるべきです。

于 2009-06-10T04:25:32.917 に答える
5
strncpy(new_product->product_name, name, sizeof(new_product->product_name));

文字列がサイズよりも長い場合、正しく終了しません。

于 2009-06-10T04:24:44.443 に答える
4

なぜnew_productグローバルであるべきなのか、そうであってはならないあらゆる理由があるのか​​ はわかりません。

于 2009-06-10T06:01:22.197 に答える
3

正しい方向に進んでいるように見えますが、問題があります。グローバル変数を削除し、代わりに関数に渡す list_t 構造体 (head と tail を含む) を用意します。他の人が指摘したように、(たとえば) node_t 型と void* データ ポインターを使用して、リストをジェネリックにすることもできます。

通常、プッシュとポップは、任意の場所ではなく、最初にアイテムを追加または削除することを指すために使用されます (あなたが行うように)。これは単なる命名の問題です。

代わりに product_name char *product_name を使用すると、長さの制限と strncpy の必要性を取り除くことができます。呼び出し元に文字列を割り当ててもらい、clean_up で解放するだけです。

メニューの読みやすさを向上させるために、列挙型の使用を検討できます。「これが最初のノードにあるかどうかを確認する - ヘッドから削除する」(テールも同様) の場合は、コードを比較するのではなく、ヘッドと製品を比較する必要があります。

「tail = previous」の後、tail->next を NULL に設定する必要があります。

于 2009-06-10T07:49:26.160 に答える
2

goldPseudoとthaggie/Stevenによって提起された問題に同意します。

push()、に置き換えstrncpy()strlcpy()、宛先文字列が常にNULで終了するようにします。

で、ステートメントcleanup()を削除することをお勧めしますexit(0);-あなたはそれを必要としません。サブルーチン内からプログラムを終了することは、一般的に最善の方法ではありません。

最初の単一リンクリストの作成から1つの教訓を取り除く必要があります。つまり、単一リンクリストは、一般的に、現実の世界ではあまり役に立ちません。理由は次のとおりです。

  • それらは操作するのが難しすぎます。pop()サブルーチンの複雑さを見てください。
  • リストから要素を取得するたびにリストの先頭から開始する必要があるため、比較的低速です。

ここで、最初の二重リンクリストを作成してみてください。二重にリンクされたリストは実装がより複雑ですが、単一にリンクされたリストよりも操作が簡単です(特に要素を削除する場合)。

于 2009-06-10T05:57:24.017 に答える
1

clean_up 関数から exit(0) を呼び出す理由はありますか? ユーザーにプログラムを正しく終了させる機会を与えないため、これは潜在的に危険だと思います。

同様に、データ構造を構築するときにデータのカプセル化を使用することをお勧めします。

typedef struct
{
    int product_code;
    char product_name[128];
    int product_cost;
    list_node *next;
} list_node;

typedef struct
{
   list_node* head;
   list_node* tail;
   list_node* current;
   int        size;
} list;

また、コードをより汎用的にするために、リストの先頭にトレイル ダミー ノードを使用することをお勧めします。

于 2009-06-10T06:08:31.183 に答える
1

通常の命名規則に従い、push と pop はスタックに関連付けられます。つまり、push() はスタックの一番上に項目を追加し (リストの末尾に追加しますが、これで問題ありません!)、pop() は返されて返されます。スタックの一番上からアイテムを削除します (リスト内の任意の場所にある名前付きアイテムを検索して削除します)。
関数名は別として、ノードのコンテンツが任意のデータへのポインター (特殊なケースでは後で product_data になります)。このようにして、リンクされたリストをあらゆるコンテンツに再利用でき、デバッグ、読み取り、および保守が容易になります。
また、グローバルなものを持たず、リストの複数のインスタンスを許可する方が良いでしょう。通常の C の方法は、データを構造体に保持し、インスタンスを各関数の最初の引数として渡すことです。

于 2009-06-10T06:32:31.777 に答える