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

My implementation of FixedSizedPriorityQueue

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

Problem

I implemented a fixed sized priority queue.
How can I improve my code?

public class FixedSizedPriorityQueue {
    private final int capacity;
    private boolean sorted = false;
    private double smallest = Double.POSITIVE_INFINITY;
    private ArrayList list;

    protected static final Comparator comparator = new Comparator() {
        @Override
        public int compare(Double o1, Double o2) {
            return o2.compareTo(o1);
        }
    };

    public FixedSizedPriorityQueue(int capacity) {
        this.capacity = capacity;
        list = new ArrayList(capacity);
    }

    public void add(double value) {
        if (list.size()  smallest) {
                if (!sorted) {
                    Collections.sort(list, comparator);
                    sorted = true;
                }
                list.set(capacity - 1, value);
                if (list.get(capacity - 2) > value) {
                    smallest = value;
                } else {
                    smallest = list.get(capacity - 2);
                    sorted = false;
                }
            }
        }
    }

    public int getSize() {
        return list.size();
    }

    public Double poll() {
        if (list.size() == 0) return null;
        if (!sorted) {
            Collections.sort(list, comparator);
            sorted = true;
        }
        Double value = list.get(0);
        list.remove(0);
        if (list.size() == 0) smallest = Double.POSITIVE_INFINITY;
        return value;
    }

    public void print() {
        for (int i = 0; i < list.size(); i++) {
            System.out.println(i + ": " + list.get(i));
        }
        System.out.println("Smallest: " + smallest);
    }
}

Solution

-
The following is used multiple times and it could be extracted out:

if (!sorted) {
    Collections.sort(list, comparator);
    sorted = true;
}


to a helper method:

private void sortIfNeccessary() {
    if (sorted) {
        return;
    }
    Collections.sort(list, comparator);
    sorted = true;
}


Note the guard clause.

-
List.remove returns the removed elements.

Double value = list.get(0);
list.remove(0);


The code above could be simplified to this:

final Double value = list.remove(0);


-
The comparator could be substituted with the reverseOrder method/comparator:

Collections.sort(list, Collections.reverseOrder());


-
The list could be final. final helps readers, because they know that the reference always points to the same instance and it doesn't change later. https://softwareengineering.stackexchange.com/questions/115690/why-declare-final-variables-inside-methods but you can find other questions on Programmers.SE in the topic.

-
Furthermore, its type could be only List instead of ArrayList. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

-
The print method violates the single responsibility principle. Other clients may want to write the elements to a log file or a network socket so you shouldn't mix the queue logic with the printing. I'd consider creating a List getElements method (which would return a copy or unmodifiable version of list to make sure that malicious clients does not modify the inner state of the queue) or providing an iterator to clients and move the print method to another class.

-
Does it make sense to create a queue with negative capacity? If not, check it and throw an IllegalArgumentException.
Actually, it does not work with capacities less than 2. It throws a java.lang.ArrayIndexOutOfBoundsException: -1 when the capacity is 1 because of the list.get(capacity - 2) call. (Effective Java, Second Edition, Item 38: Check parameters for validity)

public FixedSizedPriorityQueue(final int capacity) {
    if (capacity < 2) {
        throw new IllegalArgumentException("Illegal capacity: " + capacity);
    }
    ...
}


-
Instead of

if (list.size() == 0) ...


you could use

if (list.isEmpty()) ...


It's the same but easier to read.

Code Snippets

if (!sorted) {
    Collections.sort(list, comparator);
    sorted = true;
}
private void sortIfNeccessary() {
    if (sorted) {
        return;
    }
    Collections.sort(list, comparator);
    sorted = true;
}
Double value = list.get(0);
list.remove(0);
final Double value = list.remove(0);
Collections.sort(list, Collections.reverseOrder());

Context

StackExchange Code Review Q#15683, answer score: 7

Revisions (0)

No revisions yet.