patternjavaMinor
My implementation of FixedSizedPriorityQueue
Viewed 0 times
implementationfixedsizedpriorityqueuestackoverflow
Problem
I implemented a fixed sized priority queue.
How can I improve my code?
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:
to a helper method:
Note the guard clause.
-
The code above could be simplified to this:
-
The comparator could be substituted with the
-
The
-
Furthermore, its type could be only
-
The
-
Does it make sense to create a queue with negative capacity? If not, check it and throw an
Actually, it does not work with capacities less than 2. It throws a
-
Instead of
you could use
It's the same but easier to read.
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.