Single list

Question

I created a single linked list. Everything is working fine.

I just want to know if I did anything potentially dangerous in my code. I really like the code fragments that excite me, this is my push, pop and cleaning. Parts of the code are intended to interact with the user, so this is not very important (I sent it so that it is more clear what I am doing). Just a linked list app.

Thanks so much for any suggestions, as this is my first attempt.

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

Answer 1

From pop ()

  if(head->product_code == code) { temp = head; head = head->next; free(temp); // Finished Deleting product return; } 

If there is only one element, the head and tail will point to the same node. However, if you place this one element, the head will be adjusted, but the tail will still point to the free'd node. This will leave a bad pointer, which could cause your computer to explode.

Addition . Similarly, "new_product" will hang out if you ever pull out the last node that was clicked, and clean_up () will also leave a tail pointer. Even if the provided code sample will never play them after they are free, dangling pointers in C code should always be considered as "potentially dangerous."

Answer 2

 strncpy(new_product->product_name, name, sizeof(new_product->product_name)); 

if the string is longer than your size, it will not complete correctly.

Answer 3

I see no reason why new_product should be global and for any reason why it should not be.

Answer 4

You seem to be on the right track, but there are problems. I would delete the global variables and instead have a list_t structure (containing the head and tail) that you passed to the functions. As others have noted, you can also create a general list using (for example) the node_t type and the void * data pointer.

Usually push and pop are used to refer to adding or removing an element at the beginning, and not to an arbitrary location (like yours); it's just a naming question.

If instead there was a product_name char * product_name, this will allow you to remove the length limit, as well as the need for strncpy. You just want the caller to highlight the line, and then free it in clean_up.

You can use renaming to improve the readability of your menu. For "Check if this is in the first node - removal from the head" (the same for the tail), you should simply compare the head with the product, and not compare the codes.

After "tail = previous" you should set tail-> next to NULL.

Answer 5

Agree to the questions raised by goldPseudo and thaggie / Steven.

In push() replace strncpy() with strlcpy() to ensure that the target line is always NUL terminated.

In cleanup() I suggest removing the exit(0); statement exit(0); “You don't need her.” Exiting a program from a subprogram is usually not the best way to do it.

You should take one lesson from creating your first separately linked list, and that is, separate lists are usually not very useful in the real world, because:

• They are too difficult to manipulate. Just look at the complexity of your pop() routine.
• Relatively slow because you need to start at the top of the list every time you want to extract an item from the list.

Now you should try to write your first doubly linked list. While bidirectional lists are more difficult to implement, they are easier to manipulate (especially when deleting an item) than separate lists.

Answer 6

Is there a reason you are calling exit (0) from the clean_up function? I think this is potentially dangerous, since you are not giving the user the opportunity to finish the program correctly.

I also suggest you use data encapsulation when creating a data structure:

 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; 

It's also nice to use the node mannequin at the head of your list to make your code more general.

Answer 7

Following the usual named paths, push and pop are associated with stacks - i.e. push () should add an element to the top of the stack (you add to the tail of the list, which is great!) and pop () should return and remove an element from the top of the stack (you look for a named element anywhere in the list and delete it.)
The names of the functions aside, I would suggest a more general (abstract) implementation of the list, where the contents of the node is a pointer to arbitrary data (which in your particular case will be product_data later). This way, your linked list can be reused for any content, and it's easier to debug, read, and maintain.
It would also be better not to have the material global, but rather allow multiple instances of the list. The usual way of C is to store the data in a structure and then pass the instance as the first argument to each function.