finding kth largest element in an array implemented bag

Question

We have a collection of Comparables held in a bag and have to find the kth largest element. I copied the collection to a HashSet to remove duplicates, then converted the HashSet to an array to be sorted and consequently the kth element accessed. The code compiles, but fails the testing, and I can't figure out what's wrong. Any ideas?

public E kth(int k) {
    uniqueSet();
    Object[] uniqueArr = hashSet.toArray();
    startQuick(uniqueArr);
    return (E) uniqueArr[k - 1];
}

private void startQuick(Object[] uniqueArr) {
  int i = 0, j = uniqueArr.length;
  quickSort(uniqueArr, 0, j);
}

private void quickSort(Object[] uniqueArr, int i, int j) {
    int index = partition(uniqueArr, i, j);
    if (i < index - 1) {
        quickSort(rankBagArr, index - 1, j);
    }
    if (index < j) {
        quickSort(rankBagArr, i, index - 1);
    }
}

private int partition(Object[] uniqueArr, int i, int j) {
    E tmp;
    E pivot = (E) rankBagArr[(i + j) / 2];

    while (i <= j) {
        while (rankBagArr[i].compareTo(pivot) < 0) {
            i++;
        }
        while (rankBagArr[j].compareTo(pivot) > 0) {
            j--;
        }

        if (i <= j) {
            tmp = (E) rankBagArr[i];
            rankBagArr[i] = rankBagArr[j];
            rankBagArr[j] = tmp;
            i++;
            j--;
        }
    }
    return i;
}

Answer 1

For a start this part is highly suspect:

  if (i < index - 1)
        quickSort(rankBagArr, index-1 ,j);
  if (index < j)
        quickSort(rankBagArr, i, index-1);

Don't you mean:

  if (i < index - 1)
        quickSort(rankBagArr, i, index-1);
  if (index + 1 < j)
        quickSort(rankBagArr, index + 1, j);

?

I'm not familiar with your approach to partitioning, so I don't know whether that's correct or not. I think I understand it, and it looks okay on inspection, but it's very easy to get off-by-one errors which are hard to see without careful study.

Here's a partition method I wrote in C# recently - you should be able to translate it into Java quite easily if you want to.

private static int Partition<T>(T[] array, int left, int right,
  IComparer<T> comparer) {
  // Pivot on the rightmost element to avoid an extra swap
  T pivotValue = array[right];
  int storeIndex = left;
  for (int i = left; i < right; i++) {
    if (comparer.Compare(array[i], pivotValue) < 0) {
      Swap(array, i, storeIndex);
      storeIndex++;
    }
  }
  Swap(array, right, storeIndex);
  return storeIndex;
}

static void Swap<T>(T[] array, int x, int y) {
  T tmp = array[x];
  array[x] = array[y];
  array[y] = tmp;
}

Any reason for not just using Arrays.sort though?

Answer 2

If you want to solve the problem by sorting, then

1. Use sorting methods from API (Arrays.sort or Collections.sort). Reinventing the wheel is pointless.
2. Sort contents of your collection once, not every time you look for k-th element.

The quicksort partitioning is good for finding k-th element without sorting entire collection - you partition, if lowest range is larger then k, you recurrently go with partition to lower range, if it's smaller then k, you go to higher range and look for (k - size of lower range)-th element. It has better complexity than sorting whole collection. You can read more about it here

Anyway, your methods have parameter named uniqueArr, but some operations you perform on rankBagArr. Is it a typo? There is no definition of rankBagArr in your code.

Answer 3

May you could have a bit less of manipulations (and improve performance), and correct the default you are seeing...

Starting with a List (ArrayList), you could ask to sort it (using the comparator, and Collections.sort(list)). Then you could loop down and:

• memorizing the last element
• if you find the new element is not equals, increment a counter
• when your counter reaches the k value, the current element is your target