Count occurrences of a String in a String[] by a Map

Question

The following method is supposed to count the occurrences of each item in the given set:

void groupBy(String[] stuff)  {
LinkedHashMap<String, AtomicInteger> A = new LinkedHashMap<String, AtomicInteger>();

final AtomicInteger one = new AtomicInteger(1);
AtomicInteger count;

for (String key:stuff)  {
    count = A.get(key);
    if (count==null) A.put(key, one);
        else System.out.println("Previous value  :"+A.put(key, new AtomicInteger(count.incrementAndGet())));
}

  Set set = A.entrySet();
  Iterator ii = set.iterator();

  while(ii.hasNext()) {
     Map.Entry me = (Map.Entry)ii.next();
     System.out.print(me.getKey() + ": ");
     System.out.println(me.getValue());
  }
}

So, if I run it on param

String a[] = {"AAA", "A", "AA", "B", "A", "AAA"};

i'm supposed to get

Previous value :1
Previous value :1
AAA: 2
A: 2
AA: 1
B: 1

But, what i'm getting is

Previous value :2
Previous value :3
AAA: 3
A: 2
AA: 3
B: 3

the values in the hash are updated beyond what i intend to do and i havent an idea how.

help appreciated.

Any specific reason you used `AtomicInteger` ? You have made simple program a real complicated issue. — Prateek, Nov 13 '13 at 00:46
The only reason to use `AtomicInteger` would be if this was in a multi-threaded program. Otherwise it is totally unneeded and overly complicated. Is that your use-case? And if it is, you are using `AtomicInteger` incorrectly anyway. When you increment you should not be creating a new `AtomicInteger`. — Jim Garrison, Nov 13 '13 at 00:48
`AtomicInteger` doesn't make sense unless your program is multithreaded and you want to make sure the whole updation process happens atomically. So, weigh your options carefully before you write your code. — Prateek, Nov 13 '13 at 00:50
The `AtomicInteger` is here not the major problem. However, the way how the OP is using it is indeed overcomplicated and not well thought out. This can be done much simpler without the need to recreate a whole new instance on every single increment. — BalusC, Nov 13 '13 at 00:50
as the map is getting created inside the method, so you don't need to use any stuff related to multithreading. Because it will be assigned in the thread stack. simple integer will do. — Trying, Nov 13 '13 at 00:50

score 2 · Answer 1 · edited Nov 13 '13 at 01:05

You appear to be accidentally reusing the same AtomicInteger value with different keys. When you place an AtomicInteger into the map, it can be re-used when you call get.

Here's what's going on:

Input: AAA

It doesn't exist yet, so one is placed in the map.

Input: A

It doesn't exist yet, so one is placed in the map. There are now two references to one in the map.

Input: AA

It doesn't exist yet, so one is placed in the map. There are now three references to one in the map.

Input: B

It doesn't exist yet, so one is placed in the map. There are now four references to one in the map.

Input: A

A already exists, so the value is retrieved. Now count refers to the same object as one! count is incremented, but a copy is made. Now AAA, AA, and B are still mapped to the original AtomicInteger, but it's now wrong; it's 2. However, A refers to a second AtomicInteger, which is correct at 2.

Input: AAA

AAA already exists, so the value is retrieved. Now count refers to the same object as one, again! count is incremented, but a copy is made. Now, AA and B are still mapped to the original AtomicInteger, but it's now wrong; it's 3. However, A refers to the second AtomicInteger still, and it's still correct at 2. Additionally, AAA refers to a third AtomicInteger, but it's still wrong at 3.

Solution

Change when you create new AtomicInteger objects, and just increment when you don't need new ones; they're mutable.

for (String key : stuff)  {
    AtomicInteger count = A.get(key);
    if (count == null)
        A.put(key, new AtomicInteger(1));
    else
        count.incrementAndGet();  // Modifies the object referred to in the map.
}

You can easily switch the accepted answer to the answer which helped the most in actually understanding and solving the concrete problem. — BalusC, Nov 13 '13 at 01:05

Iłya Bursov · Accepted Answer · 2013-11-13T00:57:03.297

0

You have problem at A.put(key, one); change it to A.put(key, new AtomicInteger(1)); and this will work

also, there is really no sense in AtomicInteger, you can rewrite code as:

LinkedHashMap<String, Long> A = new LinkedHashMap<String, Long>(stuff.length);
for (String key: stuff)
    A.put(key, (A.containsKey(key)?A.get(key):0L)+1L);

edited Nov 13 '13 at 00:57

answered Nov 13 '13 at 00:49

Iłya Bursov

23,342
4
33
57

Count occurrences of a String in a String[] by a Map

2 Answers2