-2

I'm new to algorithms, I've been trying to get the merge sort work, but it just won't give the right output. No compilation errors, but I guess it's just flawed somewhere, showing random values in output as the sorted array.

void merge_sort(int[], int, int);
void merge(int[], int, int, int);
void printarray(int[], int);

int main() {
    int Arr[100], num_of_elements;
    cout << "Enter the number of elements (max 100): ";
    cin >> num_of_elements;
    cout << "Enter array elements: \n";
    for (int i = 0;i < num_of_elements;++i)
        cin >> Arr[i];
    merge_sort(Arr, 0, num_of_elements - 1);
    cout << "\nAfter Sorting (by Merge Sort):\n";
    printarray(Arr, num_of_elements);
    cout << endl;
    return 0;
}

void merge_sort(int arr[], int left, int right) {
    if (left < right) {
        int mid = (left + right) / 2;
        merge_sort(arr, left, mid);
        merge_sort(arr, mid + 1, right);
        merge(arr, left, mid, right);
    }  
}

void merge(int arr[], int left, int mid, int right) {
    int i, j, k;

    /* Calculate the lengths of the subarrays and copy the elements into them */
    int lenght_left = mid - left + 1;
    int length_right = right - mid;
    int *leftarray = new int[lenght_left];
    int *rightarray = new int[length_right];
    for (i = 0;i < lenght_left;++i)
        leftarray[i] = arr[left + i];
    for (j = 0;j < length_right;++j)
        rightarray[j] = arr[mid + 1 + j];

    /* Reordering the elements in the original array */
    for (k = left, i = 0, j = 0;k <= right;++k) {
        if (leftarray[i] <= rightarray[j])
            arr[k] = leftarray[i++];
        else
            arr[k] = rightarray[j++];
    }  

    /* Copy remaining elements into the array */
    while (i < lenght_left)
        arr[k] = leftarray[i++];
    while (j < length_right)
        arr[k] = rightarray[j++];
    delete[](leftarray);
    delete[](rightarray);
}

void printarray(int arr[], int num) {
    cout << "Displaying Elements in array: \n";
    for (int i = 0;i < num;i++)
        cout << arr[i] << "  ";
}
Gineet Mehta
  • 53
  • 1
  • 1
  • 8
  • When creating a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve) it's important to actually make it *complete*, and show how the functions you have are used, and what input you pass to them as well as the expected and actual output. Also please [read about how to ask good questions](http://stackoverflow.com/help/how-to-ask). – Some programmer dude Sep 17 '16 at 11:10
  • The right tool to solve such problems is your debugger. You should step through your code line-by-line *before* asking on Stack Overflow. For more help, please read [How to debug small programs (by Eric Lippert)](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). At a minimum, you should \[edit] your question to include a [Minimal, Complete, and Verifiable](http://stackoverflow.com/help/mcve) example that reproduces your problem, along with the observations you made in the debugger. – πάντα ῥεῖ Sep 17 '16 at 11:17
  • What do you mean by `showing random values in output as the sorted array.`? Do you mean that the array doesn't get sorted at all, gets partially sorted or it gets filled with garbage data? If the last, then you have a memory issue somewhere (=> debugger). Can you also show us the call of your merge sort function (including how you initialize your array and pass it to you function)? – rbaleksandar Sep 17 '16 at 21:09
  • Although this doesn't have much to do with your question, but please learn how to properly code in C++. You're basically writing code in C, just replacing `printf` with `cout` and `malloc` with `new`, and then calling it C++. That's so wrong on so many levels though. No offense, just a personal advice, but you really should learn the proper C++ way of doing things, like using vectors, STL algorithms, templates, etc. These are essential if you want to write quality code. C++ is **not** C. – notadam Sep 18 '16 at 19:46
  • @adam10603 I appreciate the advice, I'm just learning C++ right now, haven't have gotten to templates and everything already. Would you help with some reference links from where I can catch up and become better and more C++ inclined? I'd appreciate it a lot! :D – Gineet Mehta Sep 18 '16 at 22:45
  • Get some [good book of c++](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) – Ahmad Khan Sep 20 '16 at 07:25

2 Answers2

0

In your merge function:

/* Reordering the elements in the original array */
for (k = left, i = 0, j = 0; k <= right; ++k) {
//                           ^^^^^^^^^^^
// It should be i < lenght_left && j < length_right
    if (leftarray[i] <= rightarray[j])
        arr[k] = leftarray[i++];
    else
        arr[k] = rightarray[j++];
}

The condition you're using to control the loop is not right, the condition should be with respect to your leftarray and rightarray, it should be, that when either of the arrays reach to their end, so change your condition to i < lenght_left && j < length_right.

And when copying the remaining elements in the same function:

/* Copy remaining elements into the array */
while (i < lenght_left)
    arr[k] = leftarray[i++];
//     ^^^
while (j < length_right)
    arr[k] = rightarray[j++];
//     ^^^

Here, you forgot to increment k, change them to k++.

Ahmad Khan
  • 2,655
  • 19
  • 25
0

There are couple of mistakes you are making:

  1. You need to check that you don't go over the length of the array when you're merging

    Namely instead of:

    for (k = left, i = 0, j = 0;k <= right;++k)
    

    You should have:

    for (k = left, i = 0, j = 0;k <= right && i <lenght_left && j<length_right;++k)
    
  2. You forget to increase the array counter when adding the leftover elements. Namely:

    while (i < lenght_left)
        arr[k] = leftarray[i++];
    while (j < length_right)
        arr[k] = rightarray[j++];
    

    You should have:

    while (i < lenght_left)
        arr[k++] = leftarray[i++];
    while (j < length_right)
        arr[k++] = rightarray[j++];
    
Ahmad Khan
  • 2,655
  • 19
  • 25
JukesOnYou
  • 263
  • 2
  • 10
  • Thanks! It worked. Although I'm just wondering why not checking **i** and **j** to be less than respective lengths is needed. The loop for **k** should only run for as many elements are there in the subarray in the merge() method? – Gineet Mehta Sep 17 '16 at 11:55
  • can you answer it? – Gineet Mehta Sep 18 '16 at 09:15
  • Think of the case left part is: 10 11 12 and right part is 1 2 3. Then what will happen is that you'll add 10 11 12 in the arr adn you'll check the offset of left array which will be out of bounds. – JukesOnYou Sep 19 '16 at 09:41
  • But then just this condition `i – Ahmad Khan Sep 20 '16 at 07:23