HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaModerate

Merge sort for a List<T>

Submitted by: @import:stackexchange-codereview··
0
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,

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.