snippetjavaModerate
QuickSort of Comparable[]
Viewed 0 times
comparablequicksortstackoverflow
Problem
Here is the code for the QuickSort class that I created.
The code passed some simple tests (including duplicate values). I wonder is any bug in my code? Anywhere to improve?
public class QuickSort {
public static void sort(Comparable[] a) {
quicksort(a, 0, a.length-1);
}
private static void quicksort(Comparable[] a, int lo, int hi) {
if(lo >= hi) return;
int pi = partition(a, lo, hi);
quicksort(a, lo, pi-1);
quicksort(a, pi+1, hi);
}
private static int partition(Comparable[] a, int lo, int hi) {
int i = lo + 1;
int j = hi;
while(i 0) {
j--;
}
else if(j < i) {
break;
}
else
exchange(a, i, j);
}
exchange(a, lo, j);
return j;
}
private static void exchange(Object[] a, int i, int j) {
Object tmp = a[i];
a[i] = a[j];
a[j] = tmp;
}
}The code passed some simple tests (including duplicate values). I wonder is any bug in my code? Anywhere to improve?
Solution
Javadoc on every method would be nice.
Change signature to
Standard method to specify ranges is from inclusive to exclusive.
Why not "high" and "low"?
Your code never invokes quicksort with
You choose the first element as pivotal. Choosing as a pivot the element at a constant position allows construction of an antitest, an array on which your sort works in O(n^2). In your particular case, the antitest is pretty simple - an array in descending order, e.g. (5, 4, 3, 2, 1).
It's better to name it swap.
Change signature to
> void sort(T[] a).public static void sort(Comparable[] a) {Standard method to specify ranges is from inclusive to exclusive.
quicksort(a, 0, a.length - 1);
}
private static void quicksort(Comparable[] a, int lo, int hi) {Why not "high" and "low"?
if (lo >= hi) {Your code never invokes quicksort with
lo > hi. You'd better throw an IllegalArgument exception in this case.return;
}
int pi = partition(a, lo, hi);
quicksort(a, lo, pi - 1);
quicksort(a, pi + 1, hi);
}
private static int partition(Comparable[] a, int lo, int hi) {
int i = lo + 1;
int j = hi;You choose the first element as pivotal. Choosing as a pivot the element at a constant position allows construction of an antitest, an array on which your sort works in O(n^2). In your particular case, the antitest is pretty simple - an array in descending order, e.g. (5, 4, 3, 2, 1).
while (i 0) {
j--;
} else if (j < i) {
break;
} else {
exchange(a, i, j);
}
}
exchange(a, lo, j);
return j;
}It's better to name it swap.
private static void exchange(Object[] a, int i, int j) {
Object tmp = a[i];
a[i] = a[j];
a[j] = tmp;
}Code Snippets
public static void sort(Comparable[] a) {quicksort(a, 0, a.length - 1);
}
private static void quicksort(Comparable[] a, int lo, int hi) {if (lo >= hi) {return;
}
int pi = partition(a, lo, hi);
quicksort(a, lo, pi - 1);
quicksort(a, pi + 1, hi);
}
private static int partition(Comparable[] a, int lo, int hi) {
int i = lo + 1;
int j = hi;while (i <= j) {
if (a[i].compareTo(a[lo]) <= 0) {
i++;
} else if (a[j].compareTo(a[lo]) > 0) {
j--;
} else if (j < i) {
break;
} else {
exchange(a, i, j);
}
}
exchange(a, lo, j);
return j;
}Context
StackExchange Code Review Q#42750, answer score: 10
Revisions (0)
No revisions yet.