-1
\$\begingroup\$

I wrote a code for one of the leetcode problems but I am not satisfied. I believe I could improve it. Here is my code:

class MovingAverage {

    Queue<Integer> myQueue;
    int size;

    /** Initialize your data structure here. */
    public MovingAverage(int size) {
        myQueue = new LinkedList<Integer>();
        this.size = size;

    }

    public double next(int val) {
        int sum = 0;

        if(myQueue.size() < size) {
            myQueue.add(val);
        } else {
            myQueue.remove(); // removes the head node
            myQueue.add(val);
        }

        for(Integer item : myQueue){
            sum += item;
        }

        return (double) sum / myQueue.size();
    }
}

Runtime: 58 ms
Memory 47.8 mb

Please I need your opinions and possible improvement ideas.

\$\endgroup\$
  • 6
    \$\begingroup\$ Please add a description of the challenge. There are a lot of leetcode problems. \$\endgroup\$ – Mast Jan 19 at 7:18
  • 3
    \$\begingroup\$ (Please don't call a piece of code / a sequence of statements a code - that's something else.) \$\endgroup\$ – greybeard Jan 19 at 7:51
6
\$\begingroup\$
  • Document your code. In the code.
    • what use is MovingAverage?
    • is size fixed for the lifetime of any given MovingAverage instance?
     I don't know even scrutinising the code: size is neither final nor private
    • what does next(int value) return?

  • accumulate ints into longs - even the sum of two ints can overflow.

  • program against interfaces - and give yourself and others a chance to:
    define interfaces
  • take advantage of work already done: keep the sum around and update it instead of computing it from scratch
  • don't repeat yourself (DRY): there's myQueue.add(val); in both branches of your conditional statement

Assuming size fixed:

 /** provide one running summary about
 *  <code>int</code> values specified one by one. */
interface RunningIntStatistics {
    /** Returns the next summary value
     *  given the additional input <code>value</code>.*/
    double next(int value);
}

/** moving average over the last up to <code>size</code> 
 * <code>int</code> values specified one by one.
 * <code>size</code> is specified for instantiation.
 */
@SuppressWarnings("serial")
class MovingAverage extends java.util.ArrayDeque<Integer>
implements RunningIntStatistics {
    final int size;
    long sum;

    /** Fixes <code>size</code>. */
    public MovingAverage(int size) {
        super(size);
        this.size = size;
    }

    public double next(int value) {
        // fine point: _if_ there are "extra" elements,
        // should their values get subtracted?
        while (size <= size())
            sum -= remove().longValue();

        sum += value;
        add(value);

        return (double) sum / size();
    }
}

(MovingAverage.next() "inherits" RunningIntStatistics.next()'s doc comment.)
The above is somewhat lazy:
one should use inheritance in cases of specialisation,
else composition.
Using IntSummaryStatistics still required keeping account of values to account for.

\$\endgroup\$
  • \$\begingroup\$ If so inclined, one can open code a queue limited to size using an int[]. \$\endgroup\$ – greybeard Jan 19 at 7:42
  • \$\begingroup\$ The above is "The obvious" implementation. A nit-pickish one would keep size-1 values. \$\endgroup\$ – greybeard Jan 19 at 7:58
  • \$\begingroup\$ Thank you for your answer. \$\endgroup\$ – Kemal Gulpınar Jan 27 at 2:40
1
\$\begingroup\$

Based only on the code, I have suggestions:

  1. For the variable, I suggest that you mark them private to prevent exposure; especially the collections.

       private Queue<Integer> myQueue;
       private int size;
    
  2. I think the comment on the constructor is useless.

  3. You can remove the type java.util.LinkedList in the diamond operation, in the implementation section.

    new LinkedList<>();
    

MovingAverage#next Method

  1. I suggest you invert the logic of the condition, since you add in all cases.

    if (myQueue.size() >= size) {
       myQueue.remove(); // removes the head node
    }
    
    myQueue.add(val);
    
  2. I suggest that you extract the calculus of the sum in a method.

        private int getSum() {
           int sum = 0;
    
           for (Integer item : myQueue) {
              sum += item;
           }
    
           return sum;
        }
    

Refactored code

public class MovingAverage {
   private Queue<Integer> myQueue;
   private int size;

   public MovingAverage(int size) {
      myQueue = new LinkedList<>();
      this.size = size;
   }

   public double next(int val) {

      if (myQueue.size() >= size) {
         myQueue.remove(); // removes the head node
      }

      myQueue.add(val);

      return (double) getSum() / myQueue.size();
    }

    private int getSum() {
      int sum = 0;

      for (Integer item : myQueue) {
         sum += item;
      }

      return sum;
   }
}
\$\endgroup\$
  • \$\begingroup\$ Fixed, Thanks ! \$\endgroup\$ – Doi9t Jan 19 at 15:55
  • \$\begingroup\$ Thank you for your answer. \$\endgroup\$ – Kemal Gulpınar Jan 27 at 2:40

Not the answer you're looking for? Browse other questions tagged or ask your own question.