patternjavaMinor
Circular buffer implementation for buffering TCP socket messages
Viewed 0 times
tcpbufferingcircularmessagesforimplementationsocketbuffer
Problem
I have written my own for practice and also some tweaks for my specific requirement. There is an insert which can copy a range (well array copy really) which I am hoping is more efficient than individual element insert.
Can anyone make any comments specifically as regards efficiency and code elegance. I need to use this with Java 1.6 and am getting a warning on the
```
import java.util.Arrays; //copyOf
public class CircularBuffer {
private T[] array; //storage
private int put_idx, get_idx, count; //get/put & internal num elements counter
private final int size; //size of internal array storage
public CircularBuffer(int size) {
get_idx = 0;
put_idx = 0;
count = 0;
this.size = size;
//Type safety: A generic array of T is created for a varargs parameter - investigate how to remove warning
array = newArray(size);
}
//@SafeVarargs - ok in java 1.7
static T[] newArray(int length, T... array)
{
return Arrays.copyOf(array, length);
}
void reset(){
get_idx=put_idx=count=0;
}
public boolean empty() { return count == 0; }
public boolean full() { return count == size; }
int get_count() { return count; }
//inserts a single new element
public void insert(T element)
{
if(full())
throw new ArrayIndexOutOfBoundsException("buffer full");
array[put_idx++] = element;
put_idx %= size;
++count;
}
//insert array of specified length
public void insert(T[] thearray) {
if(thearray.length + count > size)
throw new ArrayIndexOutOfBoundsException("insufficient space to insert array");
//first copy to end - contiguous array copy
int remaining = size - put_idx;
if(remaining >= thearray.length) {
System.arraycopy(thearray,
Can anyone make any comments specifically as regards efficiency and code elegance. I need to use this with Java 1.6 and am getting a warning on the
array = newArray(size); line in the constructor. But that is the only issue I am currently aware of.```
import java.util.Arrays; //copyOf
public class CircularBuffer {
private T[] array; //storage
private int put_idx, get_idx, count; //get/put & internal num elements counter
private final int size; //size of internal array storage
public CircularBuffer(int size) {
get_idx = 0;
put_idx = 0;
count = 0;
this.size = size;
//Type safety: A generic array of T is created for a varargs parameter - investigate how to remove warning
array = newArray(size);
}
//@SafeVarargs - ok in java 1.7
static T[] newArray(int length, T... array)
{
return Arrays.copyOf(array, length);
}
void reset(){
get_idx=put_idx=count=0;
}
public boolean empty() { return count == 0; }
public boolean full() { return count == size; }
int get_count() { return count; }
//inserts a single new element
public void insert(T element)
{
if(full())
throw new ArrayIndexOutOfBoundsException("buffer full");
array[put_idx++] = element;
put_idx %= size;
++count;
}
//insert array of specified length
public void insert(T[] thearray) {
if(thearray.length + count > size)
throw new ArrayIndexOutOfBoundsException("insufficient space to insert array");
//first copy to end - contiguous array copy
int remaining = size - put_idx;
if(remaining >= thearray.length) {
System.arraycopy(thearray,
Solution
A few minor notes:
-
Comments like the following are unnecessary:
You could call the field as
-
A "solution" for the warning in the constructor:
Reference: Java how to: Generic Array creation
-
-
From Code Complete, 2nd Edition, p761:
Use only one data declaration per line
[...]
It’s easier to modify declarations because each declaration is self-contained.
[...]
It’s easier to find specific variables because you can scan a single
column rather than reading each line. It’s easier to find and fix
syntax errors because the line number the compiler gives you has
only one declaration on it.
-
Setting default integer field values to zero is unnecessary in the constructor since zero is the default int value:
-
Does it make sense to create a
-
The second
-
I'd throw
-
Here are a few unit tests:
```
@Test(expected = IllegalArgumentException.class)
public void testZeroSize() {
new CircularBuffer(0);
}
@Test
public void testIsEmptyOnNewBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullOnNewBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
assertFalse(buffer.isFull());
}
@Test
public void testIsEmpty() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
assertFalse(buffer.isEmpty());
}
@Test
public void testIsFull() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertTrue(buffer.isFull());
}
@Test
public void testIsEmptyAfterReset() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.reset();
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullAfterReset() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.reset();
assertFalse(buffer.isFull());
}
@Test
public void testIsEmptyOnUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertFalse("isEmpty 1", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 2", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 3", buffer.isEmpty());
buffer.get();
assertTrue("isEmpty 4", buffer.isEmpty());
}
@Test
public void testIsFullOnUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.get();
assertFalse("isFull 1", buffer.isFull());
buffer.get();
assertFalse("isFull 2", buffer.isFull());
buffer.get();
assertFalse("isFull 3", buffer.isFull());
}
@Test
public void testInsert() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
}
@Test(expected = IllegalStateException.class)
public void testInsertFull() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.insert("d");
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.get();
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.get();
buffer.get();
}
@Test
public void testGet() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
assertEquals("a", buffer.get());
buffer.insert("b");
buffer.insert("c");
}
@Test
public void testDelayedGet() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertEquals("a", buffer.get());
}
@Test
public void testInsertAndGetTwice() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertEquals("a", buffer.get());
assertEquals("b", buffer.get());
assertEquals("c", buffer.get());
buffer.inser
-
Comments like the following are unnecessary:
private T[] array; // storageYou could call the field as
storage and remove the comment.-
A "solution" for the warning in the constructor:
@SuppressWarnings("unchecked")
private T[] newArray(int length) {
return (T[]) new Object[length];
}Reference: Java how to: Generic Array creation
-
camelCase variable and method names are more common in Java than underscored ones. (Code Conventions for the Java Programming Language, 9 - Naming Conventions)-
From Code Complete, 2nd Edition, p761:
Use only one data declaration per line
[...]
It’s easier to modify declarations because each declaration is self-contained.
[...]
It’s easier to find specific variables because you can scan a single
column rather than reading each line. It’s easier to find and fix
syntax errors because the line number the compiler gives you has
only one declaration on it.
-
Setting default integer field values to zero is unnecessary in the constructor since zero is the default int value:
getIdx = 0;
putIdx = 0;
count = 0;-
Does it make sense to create a
CircularBuffer instance with zero size? If not check it and throw an IllegalArgumentException in the constructor.-
The second
insert method might be a premature optimization. -
I'd throw
IllegalStateException instead of ArrayIndexOutOfBoundsException. The clients of the class should not know that it uses an array (or anything else) for storage.-
Here are a few unit tests:
```
@Test(expected = IllegalArgumentException.class)
public void testZeroSize() {
new CircularBuffer(0);
}
@Test
public void testIsEmptyOnNewBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullOnNewBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
assertFalse(buffer.isFull());
}
@Test
public void testIsEmpty() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
assertFalse(buffer.isEmpty());
}
@Test
public void testIsFull() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertTrue(buffer.isFull());
}
@Test
public void testIsEmptyAfterReset() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.reset();
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullAfterReset() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.reset();
assertFalse(buffer.isFull());
}
@Test
public void testIsEmptyOnUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertFalse("isEmpty 1", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 2", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 3", buffer.isEmpty());
buffer.get();
assertTrue("isEmpty 4", buffer.isEmpty());
}
@Test
public void testIsFullOnUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.get();
assertFalse("isFull 1", buffer.isFull());
buffer.get();
assertFalse("isFull 2", buffer.isFull());
buffer.get();
assertFalse("isFull 3", buffer.isFull());
}
@Test
public void testInsert() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
}
@Test(expected = IllegalStateException.class)
public void testInsertFull() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.insert("d");
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.get();
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyUsedBuffer() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.get();
buffer.get();
}
@Test
public void testGet() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
assertEquals("a", buffer.get());
buffer.insert("b");
buffer.insert("c");
}
@Test
public void testDelayedGet() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertEquals("a", buffer.get());
}
@Test
public void testInsertAndGetTwice() {
final CircularBuffer buffer = new CircularBuffer(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertEquals("a", buffer.get());
assertEquals("b", buffer.get());
assertEquals("c", buffer.get());
buffer.inser
Code Snippets
private T[] array; // storage@SuppressWarnings("unchecked")
private T[] newArray(int length) {
return (T[]) new Object[length];
}getIdx = 0;
putIdx = 0;
count = 0;@Test(expected = IllegalArgumentException.class)
public void testZeroSize() {
new CircularBuffer<String>(0);
}
@Test
public void testIsEmptyOnNewBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullOnNewBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
assertFalse(buffer.isFull());
}
@Test
public void testIsEmpty() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
assertFalse(buffer.isEmpty());
}
@Test
public void testIsFull() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertTrue(buffer.isFull());
}
@Test
public void testIsEmptyAfterReset() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.reset();
assertTrue(buffer.isEmpty());
}
@Test
public void testIsFullAfterReset() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.reset();
assertFalse(buffer.isFull());
}
@Test
public void testIsEmptyOnUsedBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
assertFalse("isEmpty 1", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 2", buffer.isEmpty());
buffer.get();
assertFalse("isEmpty 3", buffer.isEmpty());
buffer.get();
assertTrue("isEmpty 4", buffer.isEmpty());
}
@Test
public void testIsFullOnUsedBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.get();
assertFalse("isFull 1", buffer.isFull());
buffer.get();
assertFalse("isFull 2", buffer.isFull());
buffer.get();
assertFalse("isFull 3", buffer.isFull());
}
@Test
public void testInsert() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
}
@Test(expected = IllegalStateException.class)
public void testInsertFull() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.insert("b");
buffer.insert("c");
buffer.insert("d");
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.get();
}
@Test(expected = IllegalStateException.class)
public void testGetOnEmptyUsedBuffer() {
final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
buffer.insert("a");
buffer.get();
buffer.get();
}
@Test
public void testGet() {
final CircularBuffer<String> buffer = new CircularBuffer<SContext
StackExchange Code Review Q#22826, answer score: 2
Revisions (0)
No revisions yet.