snippetjavaMinor
Merge sort efficiency
Viewed 0 times
sortefficiencymerge
Problem
I've written this Java code to implement merge sort:
Every time I want to return left and right to merge into result, I've to allocate
import java.util.*;
public class MergeSort {
int[] a;
MergeSort() {
Scanner scan = new Scanner(System.in);
System.out.println("Enter the number of items: ");
a = new int[scan.nextInt()];
for (int i = 0; i = right[r]) {
result[i] = right[r];
r++;
} else if (left[l] < right[r]) {
result[i] = left[l];
l++;
}
}
if (l == left.length) {
for (; r < right.length; ++r, ++i) {
result[i] = right[r];
}
} else if (r == right.length) {
for (; l < left.length; ++l, ++i) {
result[i] = left[l];
}
}
return result;
}
public static void main(String[] args) {
MergeSort ms = new MergeSort();
int[] result = ms.mergeSort2(0, ms.a.length - 1);
System.out.println();
for (int i = 0; i < result.length; ++i) {
System.out.print(result[i] + " ");
}
}
}Every time I want to return left and right to merge into result, I've to allocate
result again (same for cases where length == 1). How can I avoid this allocation during recursion?Solution
You can use a temporary array, see for example here or here.
As this would change quite a lot, I'm not going to rewrite your code to match those examples.
But I do have some additional comments which might improve the readability and ease of use of your code:
Constructor
A constructor should only construct an object. Your constructor also gathers user input. This makes it extremely hard to test your class.
Just let
Or pass the array directly to the sort method and just use an empty constructor.
Merging
Your merging code is a bit confusing, because you are using
With this code, you can probably also better see the similarities to the code that uses a temporary array.
And to copy the rest, you could also use
Other
As this would change quite a lot, I'm not going to rewrite your code to match those examples.
But I do have some additional comments which might improve the readability and ease of use of your code:
Constructor
A constructor should only construct an object. Your constructor also gathers user input. This makes it extremely hard to test your class.
Just let
MergeSort accept an array, which you created in a ConsoleInput class (or main).Or pass the array directly to the sort method and just use an empty constructor.
Merging
Your merging code is a bit confusing, because you are using
for loops when while would be a better fit:int l = 0;
int r = 0;
int arrayPosition = 0;
while (l = right[r]) {
result[arrayPosition] = right[r];
r++;
} else {
result[arrayPosition] = left[l];
l++;
}
arrayPosition++;
}
// copy rest
while (l < left.length) {
result[arrayPosition] = left[l];
l++;
arrayPosition++;
}
while (r < right.length) {
result[arrayPosition] = right[r];
r++;
arrayPosition++;
}With this code, you can probably also better see the similarities to the code that uses a temporary array.
And to copy the rest, you could also use
arraycopy, which is probably faster:if (l == left.length) {
System.arraycopy(right, r, result, arrayPosition, right.length - r);
} else if (r == right.length) {
System.arraycopy(left, l, result, arrayPosition, left.length - l);
}Other
- naming:
mergeSort2why2? And just writebeginninginstead ofbeg, it's not that long.
private: fields should be private (unless there is a good reason to make thempublic), same with methods.
- create a
mergemethod, it makes your code easier to read.
- the caller shouldn't need to know with what parameters to call
mergeSort2. I would create a public method without arguments (or which accepts the array) which then callsmergeSort2with the correct parameters.
Code Snippets
int l = 0;
int r = 0;
int arrayPosition = 0;
while (l < left.length && r < right.length) {
if (left[l] >= right[r]) {
result[arrayPosition] = right[r];
r++;
} else {
result[arrayPosition] = left[l];
l++;
}
arrayPosition++;
}
// copy rest
while (l < left.length) {
result[arrayPosition] = left[l];
l++;
arrayPosition++;
}
while (r < right.length) {
result[arrayPosition] = right[r];
r++;
arrayPosition++;
}if (l == left.length) {
System.arraycopy(right, r, result, arrayPosition, right.length - r);
} else if (r == right.length) {
System.arraycopy(left, l, result, arrayPosition, left.length - l);
}Context
StackExchange Code Review Q#62880, answer score: 5
Revisions (0)
No revisions yet.