Единый связанный список - PullRequest
1 голос
/ 10 июня 2009

Я создал один связанный список. Все отлично работает

Я просто хочу знать, сделал ли я что-нибудь потенциально опасное в моем коде. Фрагменты кода, которые меня беспокоят, - это мой push, pop и cleanup. Части кода предназначены только для взаимодействия с пользователем, поэтому не очень важны (я все равно написал, чтобы было более понятно, что я делал) Просто приложение со связанным списком.

Большое спасибо за любые предложения, так как это моя первая попытка.

#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);
}

Ответы [ 7 ]

9 голосов
/ 10 июня 2009

из поп ()

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

                    // Finished Deleting product
                    return;
            }

В случае наличия только одного элемента, «голова» и «хвост» будут указывать на один и тот же узел. Однако, если вы вытолкнете этот один элемент, «голова» будет скорректирована, но «хвост» все равно будет указывать на свободный узел. Это оставит неверный указатель, который может привести к взрыву вашего компьютера.

Приложение : Аналогично, 'new_product' будет зависать, если вы когда-нибудь вставите последний узел, который был нажат, и clean_up () также оставит указатель 'tail' также зависшим. Даже если предоставленный образец кода никогда не будет разыменовывать их после освобождения, висящие указатели в коде C всегда должны рассматриваться как «потенциально опасные».

5 голосов
/ 10 июня 2009
strncpy(new_product->product_name, name, sizeof(new_product->product_name));

если строка длиннее, чем у вас есть размер, она не будет завершена правильно.

4 голосов
/ 10 июня 2009

Я не вижу причин, почему new_product должен быть глобальным, и всех причин, почему он не должен быть.

3 голосов
/ 10 июня 2009

Похоже, вы на правильном пути, но есть проблемы. Я бы удалил глобальные переменные и вместо этого получил бы структуру list_t (содержащую head и tail), которую вы передаете в функции. Как уже отмечали другие, вы также можете сделать список универсальным, используя (например, тип node_t и указатель данных void *).

Обычно push и pop используются для обозначения добавления или удаления элемента в начале, а не произвольного расположения (как вы делаете); это просто вопрос именования.

Если бы вместо этого у вас было имя_продукта char * имя_продукта, это позволило бы снять ограничение по длине и необходимость в strncpy Вы бы просто дали вызывающей стороне выделить строку, а затем освободить ее в clean_up.

Вы можете использовать enum для улучшения удобочитаемости вашего меню. Для «Проверьте, находится ли это в первом узле - удаление из головы» (то же самое для хвоста), вы должны просто сравнить голову с продуктом, а не сравнивать коды.

После "tail = previous" вы должны установить tail-> рядом с NULL.

2 голосов
/ 10 июня 2009

Согласитесь с вопросами, поднятыми goldPseudo и thaggie / Steven.

В push() замените strncpy() на strlcpy(), чтобы строка назначения всегда заканчивалась NUL.

В cleanup() я бы предложил удалить оператор exit(0); - он вам не нужен. Выход из программы из подпрограммы, как правило, не лучший способ.

Вы должны убрать один урок от создания своего первого односвязного списка, то есть односвязные списки, как правило, не очень полезны в реальном мире, потому что:

  • Их слишком сложно манипулировать. Просто посмотрите на сложность вашей подпрограммы pop().
  • Относительно медленно, потому что вам нужно начинать с начала списка каждый раз, когда вы хотите извлечь элемент из списка.

Теперь вы должны попытаться написать свой первый двусвязный список. Хотя двусвязные списки сложнее реализовать, ими легче манипулировать (особенно при удалении элемента), чем односвязные списки.

1 голос
/ 10 июня 2009

Следуя обычным соглашениям об именах, push и pop связаны со стеками - т.е. push () должен добавить элемент в начало стека (вы добавляете в конец списка, что нормально!), И pop () должен вернуться и удалить элемент из верхней части стека (вы ищете именованный элемент в любом месте списка и удаляете его).
Помимо имен функций, я бы предложил более общую (абстрактную) реализацию списка, где содержимое узла представляет собой указатель на произвольные данные (который в вашем особом случае будет позже product_data). Таким образом, ваш связанный список может быть повторно использован для любого контента, и его легче отлаживать, читать и поддерживать.
Также было бы лучше не иметь глобальный материал, а разрешить несколько экземпляров списка. Обычный способ C - хранить данные в структуре, а затем передавать экземпляр в качестве первого аргумента каждой функции.

1 голос
/ 10 июня 2009

Есть ли причина, по которой вы вызываете exit (0) из функции clean_up? Я думаю, что это потенциально опасно, так как вы не даете пользователю возможности правильно завершить программу.

Также я бы предложил вам использовать инкапсуляцию данных при построении вашей структуры данных:

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;

Также рекомендуется использовать фиктивный узел в начале списка, чтобы сделать ваш код более универсальным.

...