Moving average/total algorithm

I need to keep track of the last 7 days work hours in a flat file reading loop. It's being used to measure 'fatigueability' of work rosters.

Right now I have something that works, but it seems rather verbose and I'm not sure whether there's a pattern that's more succinct.

Currently, I have a Java class with a static array to hold the last x days data, then as I read through the file, I chop off the first element and move the other 6 (for a week rolling total) back by one. The processing of this static array is done in its own method ie.

/**
 * Generic rolling average/total method. Keeps adding to an array of 
 * last 'x' seen.
 * @param d Datum point you want to add/track.
 * @param i Number of rolling periods to keep track of eg. 7 = last 7 days
 *          NOT USED AT MOMENT DURING TESTING
 * @param initFlag A flag to initialize static data set back to empty.
 * @return The rolling total for i periods.
 */
private double rollingTotal(double d, boolean initFlag) {
    // Initialize running total array eg. for new Employyes
    if (initFlag) {
        runningTotal = null;
    }
    else {
        // move d+1 b开发者_运维问答ack to d eg. element 6 becomes element 5
        for (int x = 0; x< 6 ; x++) {
            runningTotal[x] = runningTotal[x+1];
        }
        // Put current datum point at end of array.
        runningTotal[6]= d;
    }
    // Always return sum of array when this method is called.
    double myTotal = 0.0;
    for (int x = 0; x<7; x++) {
        myTotal+= runningTotal[x];
    }
    System.err.print(Arrays.toString(runningTotal)+ '\n' );
    return myTotal;
}

My question: is this a reasonable design approach, or is there something blindingly obvious and simple to do this task? Thanks guys

That certainly works, but you're doing a little more work than you have to. You can avoid moving all that data around, and you can set it up so computing the next total is a matter of subtracting the oldest value, and adding the new value.

For example:

// assume that currentIndex is where you want to add the new item
// You have another value, currentTotal, that is initialized at 0.
currentTotal = currentTotal - runningTotal[currentIndex] + d;
runningTotal[currentIndex] = d;
// increment the index.
currentIndex = (currentIndex + 1) % 7;

This uses a circular buffer and keeps the currentTotal so that it's always available.

I'd say use a queue and push the new and pop the old. For keeping track of the average, you could also just subtract the popped value from the running total and add the new one (you'd need a static or instance variable or to pass the old sum in). No need to access the rest of the elements. Also, where is runningTotal being initialized if not when the initFlag is true?

private double rollingTotal(double d, boolean initFlag) {
    if(initFlag) vals = new Queue<Integer>();
    else {
        if(vals.size() == 7) // replace 7 with i.
            total -= vals.pop().intValue();
        }
        vals.push(d);
        total += d;
    }
    return total;
}

I believe Queue is abstract, so you'll need to figure out which implementation to use. I suggest a linked-list-based one.

You might try using a circular buffer instead of moving all the data with every addition:

runningTotal[nextIndex] = d;
nextIndex+=1;
if (nextIndex>=7) nextIndex = 0;

So nextIndex is always pointing to the oldest datum. You can still sum from the beginning to the end as before.

You could use an exponential weighted moving average. Its rather long to write but the code is trivial by comparison. It tends to give smoother results as well.

double previous;
static final double DAY = 1.0;
static final double WEEK = 6.0;
static final double ALPHA = DAY/WEEK;

private double movingAverage(double d) {
    return previous = ALPHA * d + (1 - ALPHA) * previous ;
}

Note: this is an optimized version of the formula

double previous;
static final double DAY = 1.0;
static final double WEEK = 6.0;
static final double ALPHA = 1 - Math.exp(-DAY/WEEK);

private double movingAverage(double d) {
    return previous = ALPHA * d + (1 - ALPHA) * previous ;
}

In this case, the later formula is more accurate and as alpha doesn't change the overhead of Math.exp isn't important. If alpha can change, and is typically small, I suggest using the first formula.

It would be easier to use an ArrayList instead of an array. Then you could just use

ArrayList<Double> runningTotal = new ArrayList<Double>();

....

runningTotal.remove(0);
runningTotal.add(d);

Why do you initialize runningTotal to null? What is its type? Where it is declared? It would do well if you put some code samples that resemble actual Java code.

Moving on, my critique would be the following: your function does too much. A function, or method, should be cohesive. More appropriately, they should do one thing and one thing only.

Worse still, what happens in your for loop when x = 5? You copy runningTotal[6] into runningTotal[5], but then you have two copies of the same value at position 5 and 6.

In your design, your function

1. moves/shuffles the items in your array
2. calculates the total
3. prints stuff to standard error
4. returns the total

It does too much.

My first suggestion is not to move stuff around in the array. Instead, implement a circular buffer and use it instead of the array. It will simplify your design. My second suggestion is to break down things into functions that are cohesive:

1. have a data structure (a circular buffer) that allows you to add to it (and that drops the oldest entry whenever it reaches its capacity.)
2. have the data structure implement an interator
3. have a function that calculates the total on the iterator (you don't care if you are calculating the total out of an array, list or circular bufer.)
4. don't call it total. Call it sum, which is what you are computing.

That's what I'd do :)

// java pseudocode below - might not compile.

// assume you have a class called CircularBuffer, of say, doubles,
public class CircularBuffer
{
  public CircularBuffer(final int capacity) {...}
  public int getSize(){ ... return # of elements in it ... }
  public add(final Double d){ ... add to the end, drop from the front if we reach capacity... }
  public Iterator<Double> iterator(){ ... gets an interator over the content of the buffer ...}
}

// somewhere else, in another class... NOT ON CircularBuffer

public class Calculator
{
  //assume none of the double values is null
  static public Double sum(final Double ... doubles )
  {
    double sum= 0;
    for( Double d : doubles )
    {
      total += d.doubleValue();
    }
    return sum;
  }

 // you can calculate other things too
 static public Double avg(final Double ... doubles ){...}
 static public Double std(final Double ... doubles ){...}
}

/// somewhere else
{
  CircularBuffer buffer = new CircularBuffer(7);

  while( readingAndReadingAndReading )
  {
    // drops oldest values as it reaches capacity
    // always keeping the latest 7 readings
    buffer.add( getLatestValueFromSomewhere() );
  }

  System.out.println( "total=" + Calculator.sum() );
  System.out.println( "average=" + Calculator.avg() );
  System.out.println( "standard deviation=" + Calculator.std() );
}

Your task is too simple and the aproach you have adopted is certainly good for the job. However, if you want to use a better design, you must get rid of all that number movement; you better use a FIFO queue and make good use of push and pop methods; that way the code wont reflect any data movement, just the two logic actions of "new data" and "remove data older than 7 days".