74

I'm trying to use the below code to calculate the average of a set of values that a user enters and display it in a jTextArea but it does not work properly. Say, a user enters 7, 4, and 5, the program displays 1 as the average when it should display 5.3

  ArrayList <Integer> marks = new ArrayList();
  Collections.addAll(marks, (Integer.parseInt(markInput.getText())));

  private void analyzeButtonActionPerformed(java.awt.event.ActionEvent evt) {
      analyzeTextArea.setText("Class average:" + calculateAverage(marks));
  }

  private int calculateAverage(List <Integer> marks) {
      int sum = 0;
      for (int i=0; i< marks.size(); i++) {
            sum += i;
      }
      return sum / marks.size();
  }

What is wrong with the code?

1
  • 9
    You aren't summing marks, you're summing the array index i. Commented May 28, 2012 at 23:50

11 Answers 11

110

With Java 8 it is a bit easier:

OptionalDouble average = marks
            .stream()
            .mapToDouble(a -> a)
            .average();

Thus your average value is average.getAsDouble()

return average.isPresent() ? average.getAsDouble() : 0; 
Sign up to request clarification or add additional context in comments.

2 Comments

average.isPresent() ? average.getAsDouble() : defaultValue can be simplified further to optional.orElse( defaultValue )
@OlegEstekhin - Shouldn't we be using mapToInt instead of mapToDouble ? Actually is mapping needed ?
84

Why use a clumsy for loop with an index when you have the enhanced for loop?

private double calculateAverage(List <Integer> marks) {
  Integer sum = 0;
  if(!marks.isEmpty()) {
    for (Integer mark : marks) {
        sum += mark;
    }
    return sum.doubleValue() / marks.size();
  }
  return sum;
}

Update: As several others have already pointed out, this becomes much simpler using Streams with Java 8 and up:

private double calculateAverage(List <Integer> marks) {
    return marks.stream()
                .mapToDouble(d -> d)
                .average()
                .orElse(0.0)
}

7 Comments

I'd check if marks.size() == 0 in the beggining, as this will divide by zero if the list if empty
I love java, but you gotta miss C#'s list.Average() function when you're doing this :p
Just a quick note, one reason to use the clumsy loop is that it is a lot faster than the so-called civilized loop. For ArrayLists, the for(int i = 0 .... ) loop is about 2x faster than using the iterator or the for (:) approach, so even though it's prettier, it's a lot slower! One tip to make it go even faster is to cache the length as follows: for (int i = 0, len = list.size(); i <len ; i++). The len=list.size() will only get executed once in the beginning of the loop, and the len cached value will be tested each time instead.
they're actually about as fast in properly done test. interestingly enough the "enhanced for loop" and the traditional for loop ends up being executed just as fast as a while(i-->0) one, despite having an extra evaluation/call per loop. this just running on se1.7, with the arraylist filled with objects having a random int as member variable and calculating that to a sum, to make the vm do actual work. enhanced loop is about as fast as iterating yourself with iterator. if you're using arraylist, no point in using the enhanced since index based gets are faster and less gc causing.
A bit off-topic here but I was using this method in Android but Android Studio told me that the for loop required an Object type like for(Object mark: marks) (I really don't know why) obviously comes out another error inside the loop "Operator '+' cannot be applied to 'java.lang.Double', 'java.lang.Object'" so I had to cast mark to Double: sum += (Double)mark;
|
43

From Java8 onward you can get the average of the values from a List as follows:

    List<Integer> intList = Arrays.asList(1,2,2,3,1,5);

    Double average = intList.stream().mapToInt(val -> val).average().orElse(0.0);

This has the advantage of having no moving parts. It can be easily adapted to work with a List of other types of object by changing the map method call.

For example with Doubles:

    List<Double> dblList = Arrays.asList(1.1,2.1,2.2,3.1,1.5,5.3);
    Double average = dblList.stream().mapToDouble(val -> val).average().orElse(0.0);

NB. mapToDouble is required because it returns a DoubleStream which has an average method, while using map does not.

or BigDecimals:

@Test
public void bigDecimalListAveragedCorrectly() {
    List<BigDecimal> bdList = Arrays.asList(valueOf(1.1),valueOf(2.1),valueOf(2.2),valueOf(3.1),valueOf(1.5),valueOf(5.3));
    Double average = bdList.stream().mapToDouble(BigDecimal::doubleValue).average().orElse(0.0);
    assertEquals(2.55, average, 0.000001);
}

using orElse(0.0) removes problems with the Optional object returned from the average being 'not present'.

10 Comments

oops - never noticed the Java8 answer above which is the same as the one I gave
in example 2, why is mapToDouble needed when the dblList contains Doubles?
@simpleuser - because mapToDouble returns a DoubleStream, which has an average method.
I do not think the third method is working (using mapToDouble(BigDecimal::doubleValue).average()). You should use BigDecimal::valueOf instead.
And actually even that, you are still wrong, since average is only working for primitive types.
|
18

Use a double for the sum, otherwise you are doing an integer division and you won't get any decimals:

private double calculateAverage(List <Integer> marks) {
    if (marks == null || marks.isEmpty()) {
        return 0;
    }

    double sum = 0;
    for (Integer mark : marks) {
        sum += mark;
    }

    return sum / marks.size();
}

or using the Java 8 stream API:

    return marks.stream().mapToInt(i -> i).average().orElse(0);

3 Comments

It would be cleaner to caste to a double just before you return so you don't get any floating point errors creep in when marks is a very large list.
concerning the Java 8 API what are the needed imports?
@eactor In the example above no additional import is necessary.
11
sum += i;

You're adding the index; you should be adding the actual item in the ArrayList:

sum += marks.get(i);

Also, to ensure the return value isn't truncated, force one operand to double and change your method signature to double:

return (double)sum / marks.size();

1 Comment

Since he's using a list, you should use sum += marks.get(i);
9

Using Guava, it gets syntactically simplified:

Stats.meanOf(numericList);

1 Comment

If you can afford to have Guava in your project, this is SOOO much better than the ugly and verbose .stream().mapToDouble(v -> v).average() garbage! Thank you
2
List.stream().mapToDouble(a->a).average()

1 Comment

Try to use code formatting and provide some context to your answer. See the other answers as examples.
2

When the number is not big, everything seems just right. But if it isn't, great caution is required to achieve correctness.

Take double as an example:

If it is not big, as others mentioned you can just try this simply:

doubles.stream().mapToDouble(d -> d).average().orElse(0.0);

However, if it's out of your control and quite big, you have to turn to BigDecimal as follows (methods in the old answers using BigDecimal actually are wrong).

doubles.stream().map(BigDecimal::valueOf).reduce(BigDecimal.ZERO, BigDecimal::add)
       .divide(BigDecimal.valueOf(doubles.size())).doubleValue();

Enclose the tests I carried out to demonstrate my point:

    @Test
    public void testAvgDouble() {
        assertEquals(5.0, getAvgBasic(Stream.of(2.0, 4.0, 6.0, 8.0)), 1E-5);
        List<Double> doubleList = new ArrayList<>(Arrays.asList(Math.pow(10, 308), Math.pow(10, 308), Math.pow(10, 308), Math.pow(10, 308)));
        // Double.MAX_VALUE = 1.7976931348623157e+308
        BigDecimal doubleSum = BigDecimal.ZERO;
        for (Double d : doubleList) {
            doubleSum =  doubleSum.add(new BigDecimal(d.toString()));
        }
        out.println(doubleSum.divide(valueOf(doubleList.size())).doubleValue());
        out.println(getAvgUsingRealBigDecimal(doubleList.stream()));
        out.println(getAvgBasic(doubleList.stream()));
        out.println(getAvgUsingFakeBigDecimal(doubleList.stream()));
    }

    private double getAvgBasic(Stream<Double> doubleStream) {
        return doubleStream.mapToDouble(d -> d).average().orElse(0.0);
    }

    private double getAvgUsingFakeBigDecimal(Stream<Double> doubleStream) {
        return doubleStream.map(BigDecimal::valueOf)
                .collect(Collectors.averagingDouble(BigDecimal::doubleValue));
    }

    private double getAvgUsingRealBigDecimal(Stream<Double> doubleStream) {
        List<Double> doubles = doubleStream.collect(Collectors.toList());
        return doubles.stream().map(BigDecimal::valueOf).reduce(BigDecimal.ZERO, BigDecimal::add)
                .divide(valueOf(doubles.size()), BigDecimal.ROUND_DOWN).doubleValue();
    }

As for Integer or Long, correspondingly you can use BigInteger similarly.

Comments

1

Correct and fast way compute average for List<Integer>:

private double calculateAverage(List<Integer> marks) {
    long sum = 0;
    for (Integer mark : marks) {
        sum += mark;
    }
    return marks.isEmpty()? 0: 1.0*sum/marks.size();
}

This solution take into account:

  • Handle overflow
  • Do not allocate memory like Java8 stream
  • Do not use slow BigDecimal

It works coorectly for List, because any list contains less that 2^31 int, and it is possible to use long as accumulator.

PS

Actually foreach allocate memory - you should use old style for() cycle in mission critical parts

Comments

1

You can use standard looping constructs or iterator/listiterator for the same :

List<Integer> list = Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8);
double sum = 0;
Iterator<Integer> iter1 = list.iterator();
while (iter1.hasNext()) {
    sum += iter1.next();
}
double average = sum / list.size();
System.out.println("Average = " + average);

If using Java 8, you could use Stream or IntSream operations for the same :

OptionalDouble avg = list.stream().mapToInt(Integer::intValue).average();
System.out.println("Average = " + avg.getAsDouble());

Reference : Calculating average of arraylist

Comments

0

Here a version which uses BigDecimal instead of double:

public static BigDecimal calculateAverage(final List<Integer> values) {
    int sum = 0;
    if (!values.isEmpty()) {
        for (final Integer v : values) {
            sum += v;
        }
        return new BigDecimal(sum).divide(new BigDecimal(values.size()), 2, RoundingMode.HALF_UP);
    }
    return BigDecimal.ZERO;
}

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.