2

Ok, so I am trying to implement a merge sort in java, but i am running into the following error in the splitting of the array:

Exception in thread "main" java.lang.IllegalArgumentException: 2 > 1
    at java.util.Arrays.copyOfRange(Arrays.java:3591)
    at MergeSortS.recMergeSort(MergeSortS.java:26)
    at MergeSortS.recMergeSort(MergeSortS.java:28)
    at MergeSortS.mergeSort(MergeSortS.java:17)
    at MergeSortM.main(MergeSortM.java:16)
Java Result: 1
BUILD SUCCESSFUL (total time: 1 second)

My code segment is as follows, please help me identify my issue here...:( I have purposely commented out recMergeSort(right) recursive call at the end because I want to correct the recMergeSort(left) call...

public void recMergeSort(int[] tempArray){
        if(tempArray.length>1){
            int mid=(tempArray.length)/2;

            int[] left=Arrays.copyOfRange(tempArray,0,mid);

            int[] right=Arrays.copyOfRange(tempArray,mid+1,tempArray.length-1);

            recMergeSort(left);
            //recMergeSort(array, right, mid+1, right.length-1);
        }
    }

EDIT Ok, so I checked another site and the javadoc on copyOfRange method requiring the following:

Parameters:
original - the array from which a range is to be copied
from - the initial index of the range to be copied, **inclusive**
to - the final index of the range to be copied, **exclusive**. (This index may lie outside the array.)

After fixing this as follows:

public void recMergeSort(int[] tempArray){
        if(tempArray.length>1){
            int mid=(tempArray.length)/2;

            int[] left=Arrays.copyOfRange(tempArray,0,mid+1);

            int[] right=Arrays.copyOfRange(tempArray,mid+1,tempArray.length);

            recMergeSort(left);
            //recMergeSort(array, right, mid+1, right.length-1);
        }
    }

I get the following error:

Exception in thread "main" java.lang.StackOverflowError

Please help me correct this issue...:(

Vladimir Vagaytsev
  • 2,871
  • 9
  • 33
  • 36
user3889963
  • 477
  • 3
  • 6
  • 17
  • 1
    There's no need to make copies of the array. Just pass the array through with new lo/mid/high indices. – Sridhar Jun 24 '15 at 07:31

5 Answers5

2

It appears that you were getting bogged down by the Arrays.copyOfRange() method. Try this code instead:

public void recMergeSort(int[] tempArray){
    if (tempArray.length > 1 ){
        int mid=(tempArray.length)/2;                                       // mid = 2
        int[] left = Arrays.copyOfRange(tempArray, 0, mid);                 // [10, 20]
        int[] right = Arrays.copyOfRange(tempArray, mid, tempArray.length); // [30, 40, 50]

        recMergeSort(left);
        //recMergeSort(array, right, mid+1, right.length-1);
    }
}

Arrays.copyOfRange(array, lowIndex, highIndex) will copy accessing from the lowIndex inclusive and the highIndex exclusive. This means that the highIndex for the left copy should be the same as the lowIndex for the right copy.

Usage:

int[] tempArray = new int[] {10, 20, 30, 40, 50};
recMergeSory(tempArray);
Tim Biegeleisen
  • 502,043
  • 27
  • 286
  • 360
2

The reason you got java.lang.StackOverflowError is because your left array will never become array of size 1. If it enters if block then it means mid minimum will be 1 as tempArray can be 2 for it to satisfy. Then when you are copying to left 0-2, you are copying all the element again to left and right is empty

1

Too many recursions I think. From Java documentation.

Thrown when a stack overflow occurs because an application recurses too deeply.


By the way: See what happens when tempArray.length equals 2.

Your code has a bug I think.

int mid=(tempArray.length)/2; //mid equals 1
//calls with params tempArray, 0, 2
int[] left=Arrays.copyOfRange(tempArray,0,mid+1); 
//calls with params tempArray, 2, 2
int[] right=Arrays.copyOfRange(tempArray,mid+1,tempArray.length);

The last line is the problem. The length is 2, the second parameter is 2 and the third parameter is 2. The length of the returned array will be 0.

Ely
  • 10,860
  • 4
  • 43
  • 64
  • The upper index in `Arrays.copyOfRange()` is _exclusive_. So if you have an array with 2 elements, you actually want to pass in 2, which will access array[1]. – Tim Biegeleisen Jun 24 '15 at 07:12
  • Both index parameters are 2 I think. Wouldn't that be a problem? Or maybe it's me missing something. I assumed the first index parameter is inclusive and since it is 2 it is out of bound. – Ely Jun 24 '15 at 07:13
  • You are missing something ^ ^. You want to use `(tempArray, 0, mid)` _not_ `(tempArray, 0, mid+1)`. – Tim Biegeleisen Jun 24 '15 at 07:14
  • That's the OP's code. I did not post a solution. I posted the bug. – Ely Jun 24 '15 at 07:15
  • Good question. I don't know really. I just notice that possible bug. – Ely Jun 24 '15 at 07:26
  • I updated the answer. Consider that you need to be aware of the depth of the recursions you do in order to avoid the exception. – Ely Jun 24 '15 at 07:48
0

The stack overflow is a problem relating to the fact that for every call to recMergeSort you are creating two new arrays. This is called log(n) times, and causes the stack to become full.

To fix it you can either increase the stack size (see: What is the maximum depth of the java call stack? ) or you can refactor your code to use in place sorting by changing the parameters of recMergeSort(int[] tempArray) to recMergeSort(int[] unSortedArray, int startIndex, int endIndex).

For clarity: first call to it would be recMergeSort(array, 0, array.length-1), which would then call recMergeSort(array, startIndex, endIndex/2) and recMergeSort(array, (endIndex/2)+1, endIndex)

Community
  • 1
  • 1
Stig Tore
  • 266
  • 1
  • 13
  • Can you back up your answer with a practical example proving your point? If you are right I will upvote big times. – Ely Jun 24 '15 at 07:22
  • Practical example would be hard, due to size limit. – Stig Tore Jun 24 '15 at 09:37
  • 1
    A math example though: The default Java stack size is 512kb. Ignoring the overhead of a stack frame and the array object overhead (such as the length final), that's 16384 32-bit ints. Taking the above code example that creates two new arrays that splits the incoming array into two and does the same for one of those; At 256 ints there would be a total of 32896 ints in arrays when it reaches the bottom of the recursion. – Stig Tore Jun 24 '15 at 10:12
0

After factoring in all the errors you pointed out, I got it to working by changing the following along with the edits on the OP:

int mid=(tempArray.length-1)/2;

Thank you all for your support. I wasted one whole day trying to figure out this mess!

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
user3889963
  • 477
  • 3
  • 6
  • 17
  • 1
    This isn't the way to go unfortunately. You are making your pivot (the center of the array) too small by one, and then you are offsetting that by using `mid + 1`. – Tim Biegeleisen Jun 24 '15 at 07:28
  • 1
    I think Tim is right. I am actually surprised that you say it is working. Can you provide us with test cases ? – Ely Jun 24 '15 at 07:30
  • I only meant compile time errors...I haven't implemented the sorting algorithm yet – user3889963 Jun 24 '15 at 07:31
  • Implement the algorithm and then open a new question. – Tim Biegeleisen Jun 24 '15 at 07:31
  • Oh, ok. Well, consider Tim's comment though. There is some work to do I think. – Ely Jun 24 '15 at 07:32
  • Sure, I will....The splitting was supposed to be the easier part in the merge sort. If it was this hard for me, I imagine sorting algorithm would need a new question...:P – user3889963 Jun 24 '15 at 07:34
  • If anyone can point me to a good reading material on recursions that has the flow of control and stack diagrams of different recursions, it would be most appreciated. – user3889963 Jun 24 '15 at 07:41