Thread: Simplifying a simple linked list with a search function.

Code:

#include <stdlib.h>
#include <string.h>
#include <stdio.h>
 
struct node
{
        int data;
        struct node *next;
};
 
typedef struct node link;
link *searchList(int value, link *head);
 
int main()
{
        struct node *first, *current, *temp;
        first = calloc(1, sizeof(struct node));
        current = first;
        int i = 0;
        for (i = 0; i < 9; i++)
        {
                temp = calloc(1, sizeof(struct node));
                temp->data = i;
                current->next = temp;
                current = current->next;
        }
        current = first->next;
        printf("Printing node values...\n");
        while (current)
        {
                printf("%d\n", current->data);
                current = current->next;
        }       
        int search = 0;
        printf("-----------------\n");
        printf("Please enter a value to search for: ");
        scanf("%d", &search);
        current = first->next;
        printf("[%d %d]\n", search, searchList(search, first)->data);
        printf("(if the function returns [(value that's not 0) 0], it means that the term you searched for was not found)\n");

        current = first->next;
        while (current)
        {
                free(current);
                current = current->next;
        }
        free(first);
        return 0;
}
 

link *searchList(int value, link *head)
{
        struct node *temp;
        temp = head;

        while(head)
        {

                if(value == head->data)
                {

                        return head;
                }
                head = head->next;
        }
        head = temp;
        return head;
}

It fills the singly-linked list with 0 through 9 and calls a function to prompt the user to search for a number. I don't see any glaring errors, I was just wondering what could be done to simplify it or if there's anything I missed.

Code:

        while (current)
        {
                free(current);
                current = current->next;
        }

This is undefined behaviour.
You assign current from next of a node that is already deleted.

You need a temporary
e.g.

Code:

        struct node * tmp;
        while (current)
        {
                tmp = current->next;
                free(current);
                current = tmp;
        }

Kurt

Thanks for the replies guys! I went with your suggestions and removed some stuff I didn't need and threw most of main() into functions.

Code:

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

// Creates the global linked list structure and defines linked list functions.
struct node {
      int data;
      struct node *next;
};
typedef struct node link;
link *searchList(int value, link *head);
link *printList(link *current);
link *populateList(link *current);
  
int main() {

      // Initialize variables.
      struct node *head, *current, *temp;
      int search = 0;  
       
      // Initialize head node of linked list.
      head = calloc(1, sizeof(struct node));
      current = head;
      
      // Calls the list populate function and resets current node.
      populateList(current);
      current = head->next;

      // Calls the list print function.
      printList(current);
      
      // Prompts the user for a value and calls the list search function.
      printf("Enter a number to search for: ");
      scanf("%d", &search);
      current = head->next;
      printf("[%d %d]\n", search, searchList(search, head)->data);
      
      // Resets current node and frees all links from the list.
      current = head->next;
      while (current) {
             temp = current->next;
             free(current);
             current = temp;
      }
      free(head);
      free(temp);
      return 0;
}
  
// Function to search list for matching number.
link *searchList(int value, link *head) {
      struct node *temp;
      temp = head;
      while(head) {
             if(value == head->data) {
                  return head;
         }
         head = head->next;
      }
      head = temp;
      free(temp);
      return head;
}

// Function to print all values in the linked list.
link *printList(link *current) {
      printf("Printing linked list...\n");
      while (current) {
             printf("%d\n", current->data);
             current = current->next;
      }
}

// Function to populate linked list with values from 0-9.
link *populateList(link *current) {
      struct node *temp;
      int i;
      for (i = 0; i <= 9; i++) {
            temp = calloc(1, sizeof(struct node));
            temp->data = i;
            current->next = temp;
            current = current->next;
      }
}

migf1, it is homework, so some of the code (like line 36, for example) I had to keep like it is.
Also, why use puts() over printf()?

Code:

link *printList(link *current);
link *populateList(link *current);

Both functions don't return a link pointer.

Code:

link *searchList(int value, link *head) {
      struct node *temp;
      temp = head;
      while(head) {
             if(value == head->data) {
                  return head;
         }
         head = head->next;
      }
      head = temp;
      free(temp);
      return head;
}

You destroy your list if the value isn't found.
Why do you think you need to free "temp"?

I've also noticed that you don't store data in the first node (the struct "head" points to). Why?

Bye, Andreas