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

Find Kth largest element in an array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
findarraylargestelementkth

Problem

I am preparing for interviews where my code would be judged based on optimizations, OOP concepts, code readability as well as my knowledge of JAVA and design patterns.

What comments will you give in code review for the below code?

This is my code for Kth Largest element in an Array.
The logic is to create a K size max heap, and then add the smallest K elements to the heap.
Total array size can be equal to INT_MAX and k can be as small as 2

import java.util.Comparator;
import java.util.PriorityQueue;

public class CustomArray {

    class DecreasingComparator implements Comparator {

        @Override
        public int compare(Long firstNumber , Long secondNumber) {
            return (int) (secondNumber - firstNumber);
        }
    }

    public long findKthLargestElement(long[] input, int k) {
        if(k  input.length) {
            throw new IllegalArgumentException("Invalid k, should be between 1 and arra length");
        }

        PriorityQueue heap = new PriorityQueue<>(k, new DecreasingComparator());

        for(int i=0; i input[i]) {
                heap.poll();
                heap.add(input[i]);
            }
        }

        return heap.poll();
    }
}

Solution

Total array size can be equal to INT_MAX and k can be as small as 2

Why not 1?

@Override
    public int compare(Long firstNumber , Long secondNumber) {
        return (int) (secondNumber - firstNumber);
    }


I found myself several time staring at such a line and being sure, there can be NPE there! But it can and I'd suggest to add explicit checks (throwing is correct, my point is to do it explicitly).

The Comparator is seriously broken:

  • casting from long to int can overflow



  • subtraction can overflow, too



Use Long.compare.

PriorityQueue heap = new PriorityQueue<>(k, new DecreasingComparator());


That makes it simple, but boxing is not free. While a few small values get cached in a cache in java.lang.Long, for others an object must be created. This takes 16 bytes and the heap contains a reference to it (4 or 8 bytes). So you need 20 or 24 bytes per long in addition to the 8 it takes in the input array.

This may make a few megabytes if you want the millionth element. There's also an allocation, but with current smart GCs it's usually no big deal.

There's no need for a decreasing comparator, as you could find the input.length+1-kth smallest element instead.

Or you could use both and select the one giving you a smaller queue.

for(int i=0; i input[i]) {
            heap.poll();
            heap.add(input[i]);
        }
    }


This is smart and avoids most of the boxing (for small k). It could use some comments and a lot of tests.

Code Snippets

@Override
    public int compare(Long firstNumber , Long secondNumber) {
        return (int) (secondNumber - firstNumber);
    }
PriorityQueue<Long> heap = new PriorityQueue<>(k, new DecreasingComparator());
for(int i=0; i<k;i++) {
        heap.add(input[i]);
    }
    for(int i=k; i<input.length;i++) {
        if(heap.peek() > input[i]) {
            heap.poll();
            heap.add(input[i]);
        }
    }

Context

StackExchange Code Review Q#92463, answer score: 3

Revisions (0)

No revisions yet.