3

I have a multimap in where my key is a String and the values are Integers. I would like to iterate through all those Integers, in order to calculate the mean value of them, for finally, just store the key and the mean value.

This is what I have written at the moment

        int visits = 0;
    for (String key : result.keys()) {
        Object[] val = result.get(key).toArray();
        for (int i=0; i<val.length; i++){
            visits+=(Integer)val[i];
        }
        visits=visits/val.length;
        result.removeAll(key);
        result.put(key, visits);
    }

But I'm getting this error

at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
at java.util.AbstractList$Itr.next(AbstractList.java:343)
at com.google.common.collect.AbstractMapBasedMultimap$Itr.next(AbstractMapBasedMultimap.java:1150)
at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:48)
at subset.calcMax.meanCalc(calcMax.java:147)
at subset.calcMax.main(calcMax.java:208)

it points to the line for (String key : result.keys()) but the error is not in this iteration, because if I delete what is in the for loop it works. So my problem is in the iteration through the values that are for each key.

I would appreciate your help.

Thanks in advance!

Amir Afghani
  • 37,814
  • 16
  • 84
  • 124
aperezra
  • 165
  • 2
  • 7
  • read this: http://www.javacodegeeks.com/2011/05/avoid-concurrentmodificationexception.html, and: http://stackoverflow.com/questions/1655362/concurrentmodificationexception-java-iterator-next – Amir Afghani Jun 05 '14 at 17:10
  • 1
    Why don't you store the result in a Map, instead of modifying the existing Multimap and store a single value? And why are you transforming the type-safe collection into a Object array, needing cast to Integer? You can iterate on the collection of Integers directly. Also, the visits should start at 0 for each key, not just for the first one. – JB Nizet Jun 05 '14 at 17:12
  • 1
    Better yet, store the results in a `Multiset` – Ray Jun 05 '14 at 18:10

1 Answers1

1

As explained in the comments, collections throw ConcurrentModificationExceptions when modified while being iterated. It's bad practice to mutate the source collection anyway, so you're better off creating a new collection and returning that.

I would write:

ImmutableMultimap<String, Integer> computeMeanVisits(Multimap<String, Integer> multimap) {
    ImmutableMultimap.Builder<String, Integer> result = ImmutableMultimap.builder();

    for (String key : multimap.keySet()) {
        Collection<Integer> values = multimap.get(key);
        result.put(key, mean(values));
    }
    return result.build();
}

int mean(Collection<Integer> values) {
    int sum = 0;
    for (Integer value : values) {
        sum += value;
    }
    return sum / values.size();
}

As an aside:

  • I don't like your use of .toArray() to iterate on the values. In Java, it's usually preferred to manipulate collections directly. Direct Array manipulations should be reserved for very specific, high performance code, or when you have to deal with bad APIs that only accept arrays. Note that in your example, using .toArray() also makes you lose genericity, forcing you to cast each value to an Integer.
  • you should use the .keySet() method instead of the keys() method, which returns a Multiset. When iterating over this Multiset, keys associated with multiple values will appear multiple times.
  • your "visits" variable is not reset to 0 before computing a new mean
Etienne Neveu
  • 12,604
  • 9
  • 36
  • 59