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

Fixed-size array-based implementation of a queue

Submitted by: @import:stackexchange-codereview··
0
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 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.