MergeSort: Is my problem from incorrect indeces or instance variables with recursion

Question

I'm currently reading CLRS while also trying to learn C++. As you can imagine everything is going great! I've been stuck on understanding MergeSort but over the last couple days have made good progress and feel like my code is almost there. P.S. Java is my strong suit so if I mix up the vocab between languages I apologize!

I imagine the problem is stemming from one of the few issues below:
1)Because I'm still new to C++ I've read that during instances it's important to use the class variables in the way I have below in order to apply the size of the array throughout your code. With that being said, I think during my recursion the fact that my r variable is a read-only constant; it is making my code go through an extra iteration of the recursion which is leading to problems.
-My solution to this was to just set that variable equal to a new one and have it adapt to the array as needed. This still lead to wrong solutions!
2)I've read numerous times to not use Arrays in C++ especially in situations such as this and to use vectors instead. Given that I haven't learned about vectors quite yet, I felt more comfortable using Arrays and believe an issue may be from incorrect coding of pointers and references in my program.
3)My final assumption also comes from reading a lot of answers to similar questions which is to not use instance variables in recursive methods. I'm pretty sure this wasn't a problem in Java and felt like this may just be a big no-no in C++ specifically.

I've seen countless classes of MergeSort and am very confident that my mergeSort & merge methods are perfect which makes me believe that this problem is probably just a lack of knowledge to C++ syntax or rules! Any comments or help would be greatly appreciated! Lastly 2 results of arrays I got quite often were [2, 4, 4, 5, 4, 4, 5, 1] & [2, 4, 5, 4, 5, 7, 1]

#include <iostream>
#include <cmath>

using namespace std;

class MergeSort
{
    static const size_t r = 8;
    int A[r];

    public:
        MergeSort() : A{2, 4, 5, 7, 1, 2, 3, 6}
        {
            printMergeSort(A, r);
            int p = 0;
            mergeSort(A, p, r);
            printMergeSort(A, r);
        }

    static void mergeSort(int *A, int p, int r)
    {
        if(p < r)
        {
            //int q = floor(((p + r) / 2)); 
            int q = ((p + r) / 2);
            mergeSort(A, p, q);
            mergeSort(A, q + 1, r);
            merge(A, p, q, r);
        }
    }

    static void merge(int *A, int q, int p, int r)
    {
        int n1 = q - p + 1;
        int n2 = r - q;
        int L[n1+1];
        int R[n2+1];
        for(int i = 0; i < n1; i++) 
        {
            L[i] = A[p + i]; 
        }
        for(int j = 0; j < n2; j++)
        {
            R[j] = A[q + j + 1]; 
        }
        //L[n1 + 1] = std::numeric_limits<int>::max();
        L[n1] = INT_MAX;
        R[n2] = INT_MAX;
        int i = 0;
        int j = 0;
        for(int k = p; k <= r; k++)
        {
            if(L[i] <= R[j])
            {
                A[k] = L[i];
                i = i + 1;
            }
            else
            {
                A[k] = R[j];
                j = j + 1;
            }
        }
    }

    static void printMergeSort(int *A, size_t r)
    {
        for(int i = 0; i < r; i++)
        {
            cout << A[i] << " ";
        }
        cout << endl;
    }
};

int main()
{
    //MergeSort();
    MergeSort x;
    return 0;
}

score 0 · Answer 1 · answered Feb 27 '21 at 00:44

You are on right track, the best approach to learning practice algorithms are implementing them in a programming language.

Regarding your question, i think the issue is in your merging function. In merging function what you want to do is to move through both smaller arrays and pick the smallest value and store it in the temporary register.

For example if you have

{2, 7} {4, 6}

you want to pick "2" first and increment the pointer pointing to beginning of first array and then pick "4" and "6", and finally "7".

I posted the corrected version of your code here with minor variable naming changes. Here are also some notes:

constructor is not a good place to call your functions. I modified your code in a way that the merge sort and printing is called in main function.

To find mid, you do not need to use floor. When you divide two integer numbers the result will be an integer as well. i.e: (3+4)/2 = 3

#include #include

using namespace std;
const int INT_MAX = 2147483647;

class MergeSort
{
    static const size_t r = 8;
    int A[r];

    public:
        MergeSort() : A{2, 4, 5, 7, 1, 2, 3, 6}
        {
            //printMergeSort(A, r);
            int p = 0;
            //mergeSort(A, p, r);
            //printMergeSort(A, r);
        }

        void mergeSort () { 
           mergeSortUtil (A, 0, r-1);
        }
    static void mergeSortUtil(int *A, int p, int r)
    {
        if(p < r)
        {
            int mid = (p + (r-p) / 2);
            mergeSortUtil(A, p, mid);
            mergeSortUtil(A, mid + 1, r);
            merge(A, p, mid, r);
        }
    }

    static void merge(int *A, int low, int mid, int high)
    {
        int left=low; 
        int right = mid+1;
        int i=low;

        int res[high-low+1];


        while (left<=mid && right <=high) {
            if(A[left] < A[right]) {
                res[i] = A[left];
                left++;
            } else {
                res[i]=A[right];
                right++;
            }
            ++i;

        }
        while (left<=mid) {
           res[i] = A[left];
           left++; 
           i++;
        }
        while (right <=high) {
           res[i]=A[right];
           right++;
           i++;
        }

        for (int i=low; i<=high; ++i) {

          A[i]=res[i];

        }

    }

     void printMergeSort()
    {
        for(int i = 0; i < r; i++)
        {
            cout << A[i] << " ";
        }
        cout << endl;
    }
};

int main() {
    //MergeSort();
    MergeSort * x = new MergeSort();
    x->printMergeSort();
    x->mergeSort();
    x->printMergeSort();
    return 0;
}

Thanks man, great explanation! Also thank you for showing me how to call methods from the main because I haven't been able to figure out the correct syntax which is why I've been calling them from the constructor. — Liebmann5, Mar 01 '21 at 20:57

score 0 · Answer 2 · answered Feb 27 '21 at 02:06

One immediate problem is calling merge(A, p, q, r), while merge takes its arguments as (int *A, int q, int p, int r). See how p and q are mixed up?

Another problem, less obvious, is the semantics of r. An initial invocation passes 8, which implies that r is one-past the end-of-range, and does not belong to the range. However,

        mergeSort(A, p, q);
        mergeSort(A, q + 1, r);

imply that q in the first invocation does belong. You should make up your mind. Usually passing one-past leads to simpler and cleaner code. Consider

        mergeSort(A, p, q);
        mergeSort(A, q, r);

and

    int n1 = q - p;
    int n2 = r - q;
    int L[n1];
    int R[n2];

(no need to strange INT_MAX assignments anymore).

Of course, the main merge loop is wrong. This was addressed in the answer above. However, to emphasize the one-past argument, consider

    while ((i < n1) && (j < n2)) {
        if(L[i] <= R[j])
        {
            A[k++] = L[i++];
        } else {
            A[k++] = R[j++];
        }
    }
    while (i < n1) {
        A[k++] = L[i++];
    }
    while (j < n2) {
        A[k++] = R[j++];
    }

Awesome, thanks for the tipped advice! Great explanation too man! — Liebmann5, Mar 01 '21 at 20:54

MergeSort: Is my problem from incorrect indeces or instance variables with recursion

2 Answers2