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

Rolling Average improvements

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

Problem

I have created a class intended to average the previous few data points submitted. It is able to change in size and has a feature to bypass the average without affecting the data contained therein. Are there any improvements I could make, or any exceptions thrown in certain situations that I haven't thought of?

```
public class RollingAverage {

private double[] data;
private boolean bypass = false;

public RollingAverage(int size) {
if (size == 1) {
bypass = true;
}

data = new double[size];
Arrays.fill(data, Integer.MIN_VALUE);
}

/**
* Adds a data point to the rolling average
*
* @param newData
* @return the new average
*/
public double addData(double newData) {
if (bypass) {
return newData;
}
boolean full = true;
for (double point : data) {
if (point == Integer.MIN_VALUE) {
full = false;
break;
}
}

if (full) {
int j = 0;
while (j < data.length - 1) {
data[j++] = data[j];
}
data[0] = newData;

int sum = 0;
for (double point : data) {
sum += point;
}
return sum / data.length;
} else {
double sum = newData;
int j = 0;
while (data[j] != Integer.MIN_VALUE) {
sum += data[j++];
}
data[j] = newData;
return sum / (j + 1);
}
}

public void changeSize(int size) {
if (size == data.length) {
return;
}

if (size == 1) {
bypass = true;
}

double[] oldData = data;
data = new double[size];

for (int i = 0; i < size; i++) {
if (i < oldData.length) {
data[i] = oldData[i];
} else {
data[i] = Integer.MIN_VA

Solution

-
On new RollingAverage(-1) it throws a NegativeArraySizeException. An IllegalArgumentException with a proper message would be better here since clients should not know that you are using arrays in the implementation.

-
On new RollingAverageImpl(1).getAverage() it throws an ArithmeticExpression. What about an IllegalStateException here?

-
If you are using Integer.MIN_VALUE as a special marker value the addData method should not accept it. Just throw an IllegalArgumentException. (I'd consider using Double.NaN here as a named constant, like EMPTY_DATA or just EMPTY.)

-
Comparing doubles with integers (like point == Integer.MIN_VALUE) might not be precise. See: Why not use Double or Float to represent currency?

-
If you calculate the new average after every addData call maybe you should cache it and use the cached value in the getAverageDouble method.

-
Actually, I'd cache the sum too, and in the addData subtract the old value from the sum (if the array is full) and add the newData to it, then divide it by the number of elements. Maybe there are better algorithms.

Context

StackExchange Code Review Q#13719, answer score: 5

Revisions (0)

No revisions yet.