debugjavaMinor
Fixed-size array-based implementation of a queue
Viewed 0 times
arraysizebasedfixedimplementationqueue
Problem
For my computer science class, I've implemented a Java version of a queue and would like to know if there's anything wrong with it. After my tests, it seems to work fine, but maybe somebody can point out what can be improved or if something is wrong.
public class Queue implements IQueue {
private E[] internalQueue;
private int currIndex;
public Queue(int size) {
internalQueue = (E[]) new Object[size];
currIndex = 0;
}
@Override
public void push(E input) {
internalQueue[currIndex] = input;
currIndex++;
}
public E[] getQueueDebug() {
return this.internalQueue;
}
@Override
public E pop() {
if (currIndex != 0) {
E toReturn = internalQueue[0];
for (int i = 1; i <= internalQueue.length - 1; i++) {
internalQueue[i - 1] = internalQueue[i];
internalQueue[i] = null;
}
currIndex--;
return toReturn;
}
return null;
}
@Override
public E top() {
if (currIndex != 0)
return internalQueue[0];
return null;
}
@Override
public boolean empty() {
return (currIndex == 0);
}
}Solution
API
The method names you chose are unusual for queues. It's good to get inspiration from the JDK. Following the example of
I suggest these renames:
Protecting internal implementation details
The
and definitely not with
It's best to test classes through their public interfaces.
Avoid exposing internal details.
Readability
The
This meaning of this condition is "not empty",
and the code would be more readable if you replaced it with a
Copying arrays
Instead of manually copying contents of an array like this:
It's more efficient to use
Performance
Moving the elements of
You could instead use two indexes to point to the first and last element in the queue, and only move elements when really necessary.
In fact, @TedHopp put it beautifully in a comment:
[...] By thinking of the array as circular (and using two indexes as suggested) it's never necessary to move elements. All it takes is some close attention to indexing to distinguish between an empty and a full queue.
Java conventions
Note that it's not a convention in Java to prefix interface names with a capital
You could use
and since the implementation is effectively an array-backed bounded queue,
you could name the implementation class
The method names you chose are unusual for queues. It's good to get inspiration from the JDK. Following the example of
Queue,I suggest these renames:
push->add
pop->poll
top->peek
empty->isEmpty
Protecting internal implementation details
The
getQueueDebug should ideally not be there at all,and definitely not with
public visibility.It's best to test classes through their public interfaces.
Avoid exposing internal details.
Readability
The
currIndex != 0 condition is used at multiple places.This meaning of this condition is "not empty",
and the code would be more readable if you replaced it with a
!isEmpty() method call.Copying arrays
Instead of manually copying contents of an array like this:
for (int i = 1; i <= internalQueue.length - 1; i++) {
internalQueue[i - 1] = internalQueue[i];
internalQueue[i] = null;
}It's more efficient to use
System.arraycopy:System.arraycopy(internalQueue, 1, internalQueue, 0, internalQueue.length - 1);Performance
Moving the elements of
internalQueue on every poll is inefficient.You could instead use two indexes to point to the first and last element in the queue, and only move elements when really necessary.
In fact, @TedHopp put it beautifully in a comment:
[...] By thinking of the array as circular (and using two indexes as suggested) it's never necessary to move elements. All it takes is some close attention to indexing to distinguish between an empty and a full queue.
Java conventions
Note that it's not a convention in Java to prefix interface names with a capital
I, as you could see already in the Queue interface of the JDK.You could use
Queue as your interface name,and since the implementation is effectively an array-backed bounded queue,
you could name the implementation class
ArrayBackedBoundedQueue, for example.Code Snippets
for (int i = 1; i <= internalQueue.length - 1; i++) {
internalQueue[i - 1] = internalQueue[i];
internalQueue[i] = null;
}System.arraycopy(internalQueue, 1, internalQueue, 0, internalQueue.length - 1);Context
StackExchange Code Review Q#153518, answer score: 8
Revisions (0)
No revisions yet.