Error in Logic of linked list

Question

I have written a code for linked list with a head node. I'm having logical errors in removing a node and finding a node by using its attribute (code) in the list. I tried debugging but it doesn't helped much. I could use some help in finding the errors. Thanks.

Errors:

1. I cannot find a product node by its code value.
2. When I try to remove some node, it seems the nodes are non-existent.

My Debugging Output: For the main program shown here, The result was "NOT FOUND". But the node with a code value "0000" exists.

main.c

#include <stdio.h>
#include <stdlib.h>
#include "products.h"

void print_product(struct product *p)
{
    printf("%s (%s) -- stock: %d, price: %lf\n",
                p->title, p->code,
                p->stock, p->price);
}

int main()
{
    struct product_list list;
    struct product *p;
    init_list(&list);
    add_product(&list, "test", "1234", 1, 0.50);
    add_product(&list, "Product 1", "0000", 0, 10);
    add_product(&list, "Long name, isn't it", "1234567890", 10, 100);
    add_product(&list, "Product 3", "9999999", 0, 20);
    p = find_product(&list, "0000");
    if (p)
        print_product(p);
    else
        printf("Not found\n");
    int i=remove_product(&list, "0000");
    printf("Removed %d items\n", i);
    delete_list(&list);
}

products.h

struct product {
    char *title;  // Name of the product
    char code[8]; // Max. 7 characters of product ID
    int stock;  // Current stock (number of units)
    double price;  // Price of a single unit
    struct product *next; // pointer to next item in linked list
};

struct product_list {
    struct product *head;
    // could have other list specific elements, like length of the list, last update time, etc.
    };

void init_list(struct product_list *list);
struct product *add_product(struct product_list *start, const char *title, const char *code,
        int stock, double price);
struct product *find_product(struct product_list *start, const char *code);
int remove_product(struct product_list *start, const char *code);
int delete_list(struct product_list *start);

products.c

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

void init_list (struct product_list *list)
{
  list->head = malloc(sizeof(struct product));
}

/* Add product */
/* Parameters:
 * start: start of the linked list
 * title, code, stock, price: to be copied as created structure content
 * 
 * Returns: pointer to the newly added node in the linked list
 */
struct product *add_product(struct product_list *start, const char *title, const char *code,
        int stock, double price) {
    while(start->head->next != NULL){
        start->head = start->head->next;
    }
    if(start->head->title == NULL){
        start->head->title = malloc(strlen(title) + 1);
        strcpy(start->head->title, title);
        strncpy(start->head->code, code, 7);
        start->head->code[7] = 0;
        start->head->stock = stock;
        start->head->price = price;
    }else{
        start->head->next = malloc(sizeof(struct product));
        start->head = start->head->next;
        start->head->title = malloc(strlen(title) + 1);
        strcpy(start->head->title, title);
        strncpy(start->head->code, code, 7);
        start->head->code[7] = 0;
        start->head->stock = stock;
        start->head->price = price;
    }
    return start->head;
}

/* Find product */
/* Parameters:
 * start: The head node
 * code: product code to be found
 * 
 * Returns: Pointer to the first instance of product, or NULL if not found
 */
struct product *find_product(struct product_list *start, const char *code) {
    while(start->head != NULL){
        if (!strcmp(start->head->code, code)) {
            return start->head;
        }else{
            start->head = start->head->next;
        }
    }
    return NULL;
}

/* Remove Product */
/* Parameters:
 * start: The head node
 * code: value to be removed
 *
 * Enough to remove first istance, no need to check for dublicates
 *
 * Returns: The number of removed items (0 or 1)
 */
int remove_product(struct product_list *start, const char *code) {
    if (!strcmp(start->head->code, code)) {
        free(start->head->title);
        free(start->head);
        start->head = start->head->next;
        return 1;
    }
    while(start->head->next !=NULL){
        if(!strcmp(start->head->next->code, code)){
            free(start->head->next->title);
            start->head->next = start->head->next->next;
            return 1;
        }else{
            start->head = start->head->next;
        }
    }
    return 0;
}

/* Delete list */
/* Parameters:
 * start: list head
 *
 * Returns: 1, when list has been deleted
 */
int delete_list(struct product_list *listhead) {
    while(listhead->head != NULL){
        free(listhead->head->title);
        listhead->head = listhead->head->next;
    }    
    return 1;
}

UPDATED CODE WITH VALGRIND RESULTS:

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

struct product {
    char *title;  // Name of the product
    char code[8]; // Max. 7 characters of product ID
    int stock;  // Current stock (number of units)
    double price;  // Price of a single unit
    struct product *next; // pointer to next item in linked list
};

struct product_list {
    struct product *head;
    // could have other list specific elements, like length of the list, last update time, etc.
    };

void init_list (struct product_list *list)
{
  list->head = malloc(sizeof(struct product));
}

/* Add product */
/* Parameters:
 * start: start of the linked list
 * title, code, stock, price: to be copied as created structure content
 * 
 * Returns: pointer to the newly added node in the linked list
 */
struct product *add_product(struct product_list *start, const char *title, const char *code,
        int stock, double price) {
    struct product_list *newStart = start;    
    while(newStart->head->next != NULL){
        newStart->head = newStart->head->next;
    }
    if(newStart->head->title == NULL){
        newStart->head->title = malloc(strlen(title) + 1);
        strcpy(newStart->head->title, title);
        strncpy(newStart->head->code, code, 7);
        newStart->head->code[7] = 0;
        newStart->head->stock = stock;
        newStart->head->price = price;
    }else{
        newStart->head->next = malloc(sizeof(struct product));
        newStart->head = newStart->head->next;
        newStart->head->title = malloc(strlen(title) + 1);
        strcpy(newStart->head->title, title);
        strncpy(newStart->head->code, code, 7);
        newStart->head->code[7] = 0;
        newStart->head->stock = stock;
        newStart->head->price = price;
    }
    return newStart->head;
}

/* Find product */
/* Parameters:
 * start: The head node
 * code: product code to be found
 * 
 * Returns: Pointer to the first instance of product, or NULL if not found
 */
struct product *find_product(struct product_list *start, const char *code) {
    struct product_list *newStart = start;
    while(newStart->head != NULL){
        if (!strcmp(newStart->head->code, code)) {
            return newStart->head;
        }else{
            newStart->head = newStart->head->next;
        }
    }
    return NULL;
}

/* Remove Product */
/* Parameters:
 * start: The head node
 * code: value to be removed
 *
 * Enough to remove first istance, no need to check for dublicates
 *
 * Returns: The number of removed items (0 or 1)
 */
int remove_product(struct product_list *start, const char *code) {
    struct product_list *newStart = start;
    if (!strcmp(newStart->head->code, code)) {
        free(newStart->head->title);
        free(newStart->head);
        newStart->head = newStart->head->next;
        return 1;
    }
    while(newStart->head->next !=NULL){
        if(!strcmp(newStart->head->next->code, code)){
            free(newStart->head->next->title);
            newStart->head->next = newStart->head->next->next;
            return 1;
        }else{
            newStart->head = newStart->head->next;
        }
    }
    return 0;
}

/* Delete list */
/* Parameters:
 * start: list head
 *
 * Returns: 1, when list has been deleted
 */
int delete_list(struct product_list *listhead) {
    while(listhead->head != NULL){
        free(listhead->head->title);
        listhead->head = listhead->head->next;
    }    
    return 1;
}

void print_product(struct product *p)
{
    printf("%s (%s) -- stock: %d, price: %lf\n",
                p->title, p->code,
                p->stock, p->price);
}

int main()
{
    struct product_list list;
    struct product *p;
    init_list(&list);
    add_product(&list, "test", "1234", 1, 0.50);
    add_product(&list, "Product 1", "0000", 0, 10);
    add_product(&list, "Long name, isn't it", "1234567890", 10, 100);
    add_product(&list, "Product 3", "9999999", 0, 20);
    p = find_product(&list, "0000");
    if (p)
        print_product(p);
    else
        printf("Not found\n");
    int i=remove_product(&list, "0000");
    printf("Removed %d items\n", i);
    delete_list(&list);
}

Valgrind Output for the Updated code:

==20484== Memcheck, a memory error detector
==20484== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==20484== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==20484== Command: test
==20484== 
==20484== 
==20484== HEAP SUMMARY:
==20484==     in use at exit: 0 bytes in 0 blocks
==20484==   total heap usage: 31 allocs, 31 frees, 2,001 bytes allocated
==20484== 
==20484== All heap blocks were freed -- no leaks are possible
==20484== 
==20484== For counts of detected and suppressed errors, rerun with: -v
==20484== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Answer 1

Your updated code has exactly the same issues:

newStart->head = newStart->head->next;

newStart is just an alias for the start pointer.

You should be iterating on the list, not just updating its head to the next value.

For example, list

A -> B -> C

will look after your first find iteration like

B -> C

You lost one element at the head by the incorrect assignment.

it should be (for example):

struct product *find_product(struct product_list *start, const char *code) {
    struct product_list *elem = start->head;
    while(elem  != NULL){
        if (!strcmp(elem->code, code)) {
            return elem;
        }else{
            elem = elem->next;
        }
    }
    return NULL;
}

Answer 2

Your assigning here effectively eliminating the element from the list :

start->head = start->head->next;

What you're looking for is probably get some pointer

product* current;
product* nextEl = current->next;

and iterating on it:

while (nextEl != null) {
if (!strcmp(nextEl->code, code))  {
   product* current->next = nextEl->next;
   free (nextEl);
    return 1;
}
else {
   current = nextEl;
   nextEl = nextEl->next;
}

}
 return 0;

In general you should avoid to work directly with head pointer, use some local pointer in order to prevent corruption (like in find or add functions)