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

Quote:

---

Originally Posted by Paul Omans

...
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.

---

It depends on how you define simplicity. For me, for example, usage of functions results in a much more simplified & readable main() ...

Code:

---

int main( void )
{
    Node *list = NULL;
    Node *p = NULL;
    int i;

    for (i=0; i < 9; i++)
    {
        if ( !list_append(&list, i) ) {
            fputs( "*** possible memory shortage\n", stderr );
            list_free( &list );
            return 1;
        }
    }

    puts("Printing node values...");
    list_print( list );

    puts("-----------------");
    printf("Please enter a value to search for: ");
    fflush( stdout );
    scanf("%d", &i);

    p = list_search( list, i );
    printf( "%d was %sfound in the list\n", i, (p ? "\0" : "not ") );

    list_free( &list );

    return 0;
}

---

It also results in a much more maintainable and scalable code.

If this is not a homework, and you are interested, I could show the implementations of the functions I used up there (although they are considered trivial and they are available all over the Internet).

The program uses a head or dummy node, but calls it "first". You could pick a better variable name for "first", such as "head" or "dummy".

I prefer to put the pointers for nodes first in structures. This would allow generic list routines to be used with multiple node types (with the proper casting). It's not needed for your search program, since it only has one node type.

Code:

---

struct node{
    struct node * next;
    int data;
}

---

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()?

Quote:

---

Originally Posted by Paul Omans

...
Also, why use puts() over printf()?

---

It is more of a nitpicking habit, there are no important differences if you only want to output a cstring. I find puts() less verbose, more convenient when a newline must be appended on the output, and in some implementations it executes faster (if my memory serves me well, most compilers optimize printf("whatverer\n") to puts("whatever") anyway).

Quote:

---

Originally Posted by migf1

I find puts() less verbose, more convenient when a newline must be appended on the output, and in some implementations it executes faster (if my memory serves me well, most compilers optimize printf("whatverer\n") to puts("whatever") anyway).

---

I find that "less verbose" and "more convenient" is really not much of a difference here. The key for me is that printf is intended for formatted output; puts is not. If you just intend to print a string and add a newline at the end, with no format specifiers, then puts describes your intention better than printf. Consequently, meeting your intention with the appropriate function may also lead to more optimal code, but that could well be just a bonus considering that I/O is relatively expensive to begin with.

searchList is now very wrong. You're returning a pointer to something that has been freed, and you subsequently dereference it in main.
searchList should not be calling free, and it does not need a temp variable either.

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