snippetjavaModerate
Merge sort for a List<T>
Viewed 0 times
sortlistformerge
Problem
Is this merge sort following good coding standards? What are some improvements I can make to make it cleaner?
public final class MergeSort {
private MergeSort() {
// prevent instantiation
}
private static Object[] temp;
public static > void sort(List array){
Object[] a = array.toArray();
temp = new Object[a.length];
sort(a, 0, a.length - 1);
ListIterator i = array.listIterator();
for (int j=0; j mid) a[k] = temp[j++];
else if (j > hi ) a[k] = temp[i++];
else if (((Comparable)temp[j]).compareTo(temp[i]) <= -1) a[k] = temp[j++];
else a[k] = temp[i++];
}
}
}Solution
-
Binary operators should always be surrounded by whitespaces(it is inconsistent in your code: sometimes you do surround them, sometimes you don't). For example,
should be
-
There should be one whitespace before an opening bracket. Again, there is an inconsistency in your code: a whitspace is present most of the time, but it is not present, for instance, here:
-
It is a good practice to write comments for each public method to describe what it does, what do its parameters stand for, what pre- and post-conditions are guaranteed, what exceptions can be thrown, is it thread-safe or not and so in. You should write comments for all public classes, too.
Binary operators should always be surrounded by whitespaces(it is inconsistent in your code: sometimes you do surround them, sometimes you don't). For example,
for (int j=0; j<a.length; j++)should be
for (int j = 0; j < a.length; j++)-
There should be one whitespace before an opening bracket. Again, there is an inconsistency in your code: a whitspace is present most of the time, but it is not present, for instance, here:
if(hi
-
A little bit about blank lines: they should be present between different methods and it is fine to have them inside one method if it is really big and you need to separate logical blocks from each other(however, too long methods should be avoided). But having a blank line after each statement is pointless and it makes your code less readable.
private static void sort(Object[] a, int lo, int hi) {
if(hi <= lo) {
return;
}
int mid = lo + (hi - lo) / 2;
sort(a, lo, mid); // sort left part
sort(a, mid + 1, hi); // sort right part
merge(a, lo, mid, hi);
}
looks better than:
private static void sort(Object[] a, int lo, int hi) {
if(hi <= lo){
return;
}
int mid = lo + (hi - lo)/2;
sort(a, lo, mid);// sort left part
sort(a, mid+1, hi); // sort right part
merge(a, lo, mid, hi);
}
-
Variable naming. There is no need to shorten words: you code will become more readable if you use names like low and high instead of lo and hi.
-
It is conventional to declare one variable at a time. I would use
int i = lo;
int j = mid + 1;
instead of int i = lo, j = mid+1;. And again, i and j` are not the best names for variables because they are not descriptive(but it can be ok if their scope is very narrow, so it is not a very big issue in this case).-
It is a good practice to write comments for each public method to describe what it does, what do its parameters stand for, what pre- and post-conditions are guaranteed, what exceptions can be thrown, is it thread-safe or not and so in. You should write comments for all public classes, too.
Code Snippets
for (int j=0; j<a.length; j++)for (int j = 0; j < a.length; j++)private static void sort(Object[] a, int lo, int hi) {
if(hi <= lo) {
return;
}
int mid = lo + (hi - lo) / 2;
sort(a, lo, mid); // sort left part
sort(a, mid + 1, hi); // sort right part
merge(a, lo, mid, hi);
}private static void sort(Object[] a, int lo, int hi) {
if(hi <= lo){
return;
}
int mid = lo + (hi - lo)/2;
sort(a, lo, mid);// sort left part
sort(a, mid+1, hi); // sort right part
merge(a, lo, mid, hi);
}int i = lo;
int j = mid + 1;Context
StackExchange Code Review Q#77971, answer score: 14
Revisions (0)
No revisions yet.