Sorted insertion in a linked list

Question

I'm trying to write some basic functions in linked list and one of them is a sorted insert. I understand what it's supposed to do but it gives me a semi sorted list. I don't know where the problem is. It does the job but some of the numbers are not in the right place. So if you could find where exactly this happens, I would be grateful.

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

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

struct node* sorted_insert(struct node* ptr, int data){
  struct node* newNode = malloc(sizeof(struct node));
  if (!newNode){
    printf("something went wrong using malloc");
  }
  newNode -> data = data;

  if(ptr->next == NULL) {
    newNode -> next = ptr; //ptr is the most recent element
    return newNode;
  } else {

    struct node* prev = ptr->next;
    struct node* current = ptr;

    if(prev->next == NULL){
      if((current -> data) < (newNode -> data)){
        newNode -> next = ptr;
        return newNode;
      } else {
        newNode -> next = ptr->next;
        ptr -> next = newNode;
        return ptr;
      }
    } else {

      while((current -> data) > data && (prev -> data) > data) {
        current = prev;
        prev = prev->next;
      }

      newNode -> next = prev;
      current -> next = newNode;
      return ptr;
    }

  }
}

struct node* insert(struct node* ptr, int data){
  struct node* newNode = malloc(sizeof(struct node));
  if (!newNode){
    printf("something went wrong using malloc");
  }
  newNode -> data = data;
  newNode -> next = ptr;
  return newNode;
}

void print(struct node* root){
  struct node* trav = root;
  while(trav->next != NULL){
    printf("%d\n", trav -> data);
    trav = trav -> next;
  }
}
int main(){
  struct node *head = NULL;
  head = malloc(sizeof(struct node));
  if (!head){
    printf("something went wrong using malloc");
  }

  head -> data = -1;
  head -> next = NULL;

  int i;
  srand(time(NULL));
  for(i = 0; i < 20; i++) {
    head = sorted_insert(head, rand()%2000);
     print(head);
     printf("\n");
  }

  //printf("number of elements %d\n", length(head));
  //print(head);
}

see sorted_insert function

sample output:

1279
1755
1295
1983
1353
1313
1924
1635
1296
1807
1263
1257
1199
771
386
359
278
231
141
45

Answer 1

The code is using a dummy head node. Usually the dummy node precedes the list, rather than being used to terminate the list. The code should not change head in the main program, and sorted_insert doesn't need to return a pointer to node, since it updates head->next, which is the actual pointer to the first node. sorted_insert should set prev = ptr and current = ptr->next. The check to insert should be newNode->data < current->data (if so, insert newNode just before current). There's no need to set head->data = -1. I don't know if there are other issues.

Example code. node changed to a typedef. The dummy node precedes the list. head is used as a local pointer in the functions. Added checks for head == NULL.

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

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

void sorted_insert(node * head, int data)
{
node * newNode;
node * curr;
    if(head == NULL)
        return;
    newNode = malloc(sizeof(node));
    if (!newNode){
        printf("something went wrong using malloc");
        return;
    }
    /* insert node, head used as previous ptr */
    newNode->data = data;
    curr = head->next;
    while(curr != NULL && curr->data <= data){
        head = curr;
        curr = curr->next;
    }
    head->next = newNode;
    newNode->next = curr;
}

void insert(node * head, int data)
{
    node * newNode;
    if(head == NULL)
        return;
    newNode = malloc(sizeof(node));
    if (!newNode){
        printf("something went wrong using malloc");
        return;
    }
    newNode->data = data;
    newNode->next = head->next;
    head->next = newNode;
}

void print(node * head)
{
    if(head == NULL)
        return;
    head = head->next;
    while(head != NULL){
        printf("%d\n", head -> data);
        head = head->next;
    }
}

void delete(node *head)
{
node * curr;
    if(head == NULL)
        return;
    curr = head->next;          /* curr = start of list */
    head->next = NULL;          /* set list to empty */
    while(curr != NULL){        /* delete nodes */
        head = curr;
        curr = curr->next;
        free(head);
    }
}

int main()
{
node dummy = {NULL, -1};
node * head = &dummy;
int i;
    srand((unsigned int)time(NULL));
    for(i = 0; i < 20; i++)
        sorted_insert(head, rand()%2000);
    print(head);
    delete(head);
    return 0;
}

Answer 2

Your sorted insert was a bit more complicated than it needed to be. Since you are using the dummy node at the end, you only have 2 conditions to test for (3 if you consider adding 1st node a condition).

1. "Is newNode->data greater than ptr->data?" If so, just add it to the front of the list and return newNode.

2. Otherwise, simply traverse the list setting current=current->next until newNode->data < current->next->data. Then just add newNode at that point.

Simplifying, and clearing up your confusing choices of prev and current pointer convention, you can simplify your sorted insert to the following:

struct node *sorted_insert (struct node *ptr, int data)
{
    struct node *current = ptr;
    struct node *newNode = malloc (sizeof (struct node));

    if (!newNode) {
        printf ("something went wrong using malloc");
    }
    newNode->data = data;
    newNode->next = NULL;       /* always initialize next ptr */

    /* first node, or newNode->data greates - add at front */
    if (current->next == NULL || newNode->data > current->data) {
        newNode->next = current;
        return newNode;
    }

    /* traverse list to sorted insertion point */
    while (current->next && newNode->data < current->next->data)
        current = current->next;

    newNode->next = current->next;
    current->next = newNode;

    return ptr;
}

Further confusing issues is the fact that you are printing the list as the values are inserted rather than printing the contents of the full list. (you may be doing this for debug, if so ignore...). To correct the problem -- use your print function as it was designed to be used -- to print the full list (except for your dummy node at the end), e.g.

srand (time (NULL));
for (i = 0; i < 20; i++)
    head = sorted_insert (head, rand () % 2000);

print (head);

Two final ancillary notes:

1. Don't printf a single character, that is what putchar is for, e.g. putchar ('\n');.

2. The proper declaration for main is type int and it therefore should return a value (note: 0 is returned if you fail to include a return statement) See: C11 Standard §5.1.2.2.1 Program startup (draft n1570). See also: What should main() return in C and C++?

Lastly, rather than printing the node->data to stdout to verify that your nodes were inserted in sort-order, get in the habit of writing and using short validation routines that simply check the order and only complain (in some meaningful way) if something wrong is found. You can use a variation on your print function to compare the previous->data > current->data and only provide output if an error is encountered, e.g.

/* silently validate sort order, output to stderr 
 * only if unsorted nodes found. return 1 on error,
 * 0 otherwise.
 */
int chkorder (struct node *root)
{
    struct node *trav = root;
    int prev = trav->data;

    for (trav = trav->next; trav->next; trav = trav->next) {
        if (prev < trav->data) {
            fprintf (stderr, "error: unsorted nodes (%d < %d)\n",
                    prev, trav->data);
            return 1;
        }
        prev = trav->data;
    }
    return 0;
}

Sure, checking 20 values by dumping to the screen is fine, but checking 20,000 that way is a bit of a problem. You can then validate your insertions with something like:

if (chkorder (head))
    fprintf (stderr, "error: sorted_insert failure.\n");
else
    printf ("all nodes inserted in descending sort order.\n");