stuck on implementation of merge sort

Question

I am implementing mergesort referencing the geeks for geeks implementation as a guide but my implementation is not working.

I have my mergesort function that divides my given array in 2 and calls mergesort on half of the list.

Within mergesort, I use a helper function that merges the sub arrays together.

I have included my 2 functions. Could someone be my second set of eyes, I am have staring at this too long to tell the difference between 1's and l's

It is running but not sorting correctly.

void merge(int arr[], int temp[], int l, int m, int r) {
  //TODO: implement merge.
  // check arr
  if (arr == NULL) {
        return;
  }

  int left = m - l + 1;
  int right = r - m;

  // copy array into temp array 
  // first half
  int i = 0;
  for (i = 0; i < left; i++) {
        temp[i] = arr[l + i];
  }
  // second half
  int j = 0;
  for (j = m + 1; i < right; j++) {
        temp[j] = arr[m + l + i];
  }

  // compare from each end inserting the lower into the next location of the real array 
  // beginning index of front sub list
  int front = 0;
  // beginning of back sub list
  int back = left;

  // index within array to insert back in 
  int index = l;
  while ((front < left) && (back < right)) {
        
        if (temp[front] <= temp[back]) {
            // temp front goes in the next array spot
            arr[index] = temp[front];
            // increase temp
            front++;
        } else {
            // back is smaller and is put back in the list first 
            arr[index] = temp[back];
            // increase back 
            back++;
        }
        // increase array index
        index++;
  }

  while (front < left) {

        arr[index] = temp[front];
        front++;
        index++;
  }

  while (back < right) {

        arr[index] = temp[back];
        back++;
        index++;
  }
}

void mergeSort(int array[], int temp[], int l, int r) {

    if (r > l) {

        // find middle point
        int middle = l + (r - l) / 2;

        // call merge on first half
        mergeSort(array, temp, l, middle);

        // call merge on second half
        mergeSort(array, temp, middle + 1, r);

        // merge the halves
        merge(array, temp, l, middle, r);
    }
}

Answer 1

There is a problem here:

  // second half
  int j = 0;
  for (j = m + 1; i < right; j++) {
        temp[j] = arr[m + l + i];
  }

You should write:

    // second half
    for (i = 0; i < right; i++) {
        temp[left + i] = arr[m + 1 + i];
    }

Could someone be my second set of eyes, I am have staring at this too long to tell the difference between 1's and l's?

This is an excellent point! The solution is to never use l as a variable name, and to simplify the mergesort implementation to remove the confusing and error prone +1 / -1 adjustments. For this you just need to use the convention where r is the index of the element after the end of the slice.

Here is a modified version:

void merge(int arr[], int temp[], int lo, int mid, int hi) {
    // check arr
    if (arr == NULL) {
          return;
    }

    int left = mid - lo;
    int right = hi - mid;

    // copy array into temp array 
    // first half
    for (int i = 0; i < left; i++) {
        temp[i] = arr[lo + i];
    }
    // second half
    for (int i = 0; i < right; i++) {
        temp[left + i] = arr[mid + i];
    }

    // compare from each end inserting the lower into the next location of the real array 
    // beginning index of front sub list
    int front = 0;
    // beginning of back sub list
    int back = left;

    // index within array to insert back in 
    int index = lo;
    while ((front < left) && (back < right)) {
        if (temp[front] <= temp[back]) {
            // temp front goes in the next array spot
            arr[index++] = temp[front++];
        } else {
            // back is smaller and is put back in the list first 
            arr[index++] = temp[back++];
        }
    }

    while (front < left) {
        arr[index++] = temp[front++];
    }

    while (back < right) {
        arr[index++] = temp[back++];
    }
}

void mergeSort(int array[], int temp[], int lo, int hi) {
    if (hi - lo >= 2) {
        // find middle point
        int mid = lo + (hi - lo) / 2;

        // call merge on first half
        mergeSort(array, temp, lo, mid);

        // call merge on second half
        mergeSort(array, temp, mid, hi);

        // merge the halves
        merge(array, temp, lo, mid, hi);
    }
}

The initial call should give 0 and the length of the array as index arguments.

Answer 2

I had some trouble understanding your merge logic.

Too many indexes/limits of the form (e.g): m - l + 1

So, I simplified things by having two temp pointers: tmp_l and tmp_r and added some length/count variables.

This allowed some of the indexes to start with 0.

Also, your middle calculation was unusual.

And, I changed the arg in the call to merge from middle to middle + 1

---

I've refactored the code and added a test suite. I've used cpp conditionals to denote old vs new code:

#if 0
// old code
#else
// new code
#endif

Anyway, here's my [working] version:

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

#ifdef DEBUG
#define dbgprt(_fmt...) \
    do { \
        printf(_fmt); \
    } while (0)
#else
#define dbgprt(_fmt...) \
    do { \
    } while (0)
#endif

void
merge(int arr[], int temp[], int l, int m, int r)
{

    if (arr == NULL) {
        return;
    }

    // get number of left elements
    int lcnt = (m - l);

    // get number of right elements
    int rcnt = (r - m) + 1;

    dbgprt("merge: BEGIN l=%d m=%d r=%d lcnt=%d rcnt=%d\n",
        l,m,r,lcnt,rcnt);

    int i = 0;

    int *tmp_l = &temp[0];
    for (i = 0; i < lcnt; i++)
        tmp_l[i] = arr[l + i];

    int *tmp_r = &temp[lcnt];
    for (i = 0; i < rcnt; i++)
        tmp_r[i] = arr[m + i];

    int front = 0;
    int back = 0;

    int index = l;

    while ((front < lcnt) && (back < rcnt)) {
        // temp front goes in the next array spot
        if (tmp_l[front] <= tmp_r[back]) {
            arr[index] = tmp_l[front];
            // increase temp
            front++;
        }

        // back is smaller and is put back in the list first
        else {
            arr[index] = tmp_r[back];
            // increase back
            back++;
        }

        // increase array index
        index++;
    }

    while (front < lcnt) {
        arr[index] = tmp_l[front];
        index++;
        front++;
    }

    while (back < rcnt) {
        arr[index] = tmp_r[back];
        index++;
        back++;
    }
}

void
mergeSort(int array[], int temp[], int l, int r)
{
    int middle;

    if (r > l) {
        // find middle point
#if 0
        middle = l + (r - 1) / 2;
#else
        middle = (l + r) / 2;
#endif

        dbgprt("msort: ENTER l=%d m=%d r=%d\n",l,middle,r);

        // call merge on first half
        mergeSort(array, temp, l, middle);

        // call merge on second half
        mergeSort(array, temp, middle + 1, r);

        // merge the halves
#if 0
        merge(array, temp, l, middle, r);
#else
        merge(array, temp, l, middle + 1, r);
#endif

        dbgprt("msort: EXIT l=%d m=%d r=%d\n",l,middle,r);
    }
}

void
dotest(int tstno)
{

    int count = (rand() % 30) + 1;
    printf("dotest: %d %d\n",tstno,count);

    int *arr = malloc(sizeof(*arr) * count);
    int *tmp = malloc(sizeof(*tmp) * count);

    for (int idx = 0;  idx < count;  ++idx) {
        arr[idx] = count - idx;
        dbgprt(" %d",arr[idx]);
    }
    dbgprt("\n");

    mergeSort(arr,tmp,0,count - 1);

    int err = 0;
    for (int idx = 0;  idx < count;  ++idx) {
        printf(" %d",arr[idx]);
        if (arr[idx] != (idx + 1)) {
            printf("?");
            err = 1;
        }
    }
    printf("\n");

    if (err)
        exit(1);

    free(arr);
    free(tmp);
}

int
main(void)
{

    for (int tstno = 1;  tstno <= 4;  ++tstno)
        dotest(tstno);

    return 0;
}

void
merge_ORIG(int arr[], int temp[], int l, int m, int r)
{

    if (arr == NULL) {
        return;
    }

    int i = 0;

    for (i = 0; i < m - l + 1; i++) {
        temp[i] = arr[l + i];
    }

    for (i = 0; i < r + 1; i++) {
        temp[i] = arr[m + l + i];
    }

    int front = l;
    int back = m + 1;
    int index = l;

    while ((front < m - l + 1) && (back < r - m)) {
        if (temp[front] <= temp[back]) {

            // temp front goes in the next array spot
            arr[index] = temp[front];
            // increase temp
            front++;

        }
        else {
            // back is smaller and is put back in the list first
            arr[index] = temp[back];
            // increase back
            back++;
        }

        // increase array index
        index++;
    }

    while (front < m - l + 1) {
        arr[index] = temp[front];
        index++;
        front++;
    }

    while (back < r - m) {
        arr[index] = temp[back];
        index++;
        back++;
    }
}