Just a lowly counter that turned out to be surprisingly complicated

While writing this review, I saw a need for a Counter object similar to Python's. It seemed like it would be easy to write such a class, but it turned out to be surprisingly complicated.

Counter.java

```
import java.util.*;

public class Counter implements Map,
Iterable> {

private final Map c = new HashMap();

/**
* Comparator to sort entries by decreasing count. In case of a tie,
* arbitrary but deterministic tie-breakers are used.
*/
private final Comparator> DESC_FREQ_COMPARATOR =
new Comparator>() {
@Override
public int compare(Map.Entry a, Map.Entry b) {
int aValue = a.getValue().intValue();
int bValue = b.getValue().intValue();
int diff;
if (0 != (diff = bValue - aValue)) {
return diff;
} else if (0 != (diff = b.hashCode() - a.hashCode())) {
return diff;
} else {
T aKey = a.getKey();
T bKey = b.getKey();
if (aKey == null && bKey == null) {
return 0;
} else if (aKey == null) {
return 1;
} else if (bKey == null) {
return -1;
} else if (bKey instanceof Comparable) {
@SuppressWarnings("unchecked")
Comparable bKeyCmp = (Comparable)bKey;
return bKeyCmp.compareTo(aKey);
} else {
return bKey.toString().compareTo(aKey.toString());
}
}
}
};

/**
* Creates an empty counter.
*/
public Counter() {}

/**
* Copy constructor.
*/
public Counter(Map counter) {
this();
this.putAll(counter);
}

/**
* Returns the number of key-value mappings in this map, including entries
* that have a zero count.
*/
@Override
publi

Is there a common Java library that serves a similar purpose, or am I
reinventing-the-wheel?

As far as I know the JRE doesn't offer this out of the box.


Are the semantics for null keys and zero counts reasonable?

Would it be typical to be counting nulls? I guess it's nice that it's possible, even though it opens the possibility for exceptions. Personally I'd disallow it.

What is reasonable is that getting a count for keys that are not present, simply returns 0.


Is it useful to implement the Map and Iterable interfaces?
Or does that unnecessarily complicate things?

Implementing the Map interface is unnecessary, it requires a lot of code that adds nothing to the intended responsibility of this class. In fact you do not fully abide to the Map interface contract. Honoring the Single Responsibility Principle mandates dropping this.

Implementing the Iterable interface is way more useful, as it exposes the results in a standard way.


I wrote many increment() and decrement() methods. Do they make the
class convenient to use, or are they too redundant?

I would extract an interface from this class that captures its API, and you can make alternative implementations later on as needed.

public interface Counter extends Iterable> {
    void increment(T key);

    void decrement(T key);

    int getCount(T key);

    Set getKeys();
}

I would even replace Map.Entry by a Counter specific interface.

Try to resist the urge to add convenience methods, You can always use the Decorator pattern to add candy as needed. In the mean time focus on making a great core implementation.


What do you think of the method signatures for mostCommon() and
mostCommon(int)? Is there too much redundancy with entrySet() and
iterator()?

These are superfluous, again decorators are the way to go in this case.


I had to use @SuppressWarnings("unchecked") in three places. How can I
avoid those warnings in the first place?

First occurrence is :

CounterImpl other = (CounterImpl)o;
Map.Entry[] a = this.mostCommon();
@SuppressWarnings("unchecked")
Map.Entry[] b = other.mostCommon();

You declare the other variable without type parameter. Rule 1 to avoiding unchecked warnings is to specify the type parameter on classes that have one. After all, the only reason you can omit them, is that this was needed for backwards compatibility when generics were introduced.

CounterImpl other = (CounterImpl)o;
Map.Entry[] a = this.mostCommon();
Map.Entry[] b = other.mostCommon();

A likewise case happens in mostCommon()

Next case :

} else if (bKey instanceof Comparable) {
    @SuppressWarnings("unchecked")
    Comparable bKeyCmp = (Comparable)bKey;

Reflection code and generics is bound to get you into trouble with unchecked warnings. In fact this cast actually isn't safe when T implements Comparable where S is a supertype of T.


I increment a count using two calls to put() of a HashMap. Is there a
more efficient way?

Yes there is. Instead of using Integer as the value in your Map, use a mutable count representation. You can easily write one yourself, or use AtomicInteger (although you may not need the thread safety). Then define :

private final Map c = new HashMap();

Now you'll only need to do a put if the key isn't in the Map yet :

public void increment(T key, int delta) {
    Integer prev = this.c.put(key, delta);
    if (!c.containsKey(key)) {
        c.put(key, new AtomicInteger());
    }
    c.get(key).addAndGet(delta);
}

In java 8 it becomes even simpler using java.util.Map#computeIfAbsent

c.computeIfAbsent(key, k -> new AtomicInteger()).addAndGet(delta);

To support entrySet() and mostCommon(), I build a SortedSet from
scratch. Is there a better data structure I could have used that could
support entrySet() and mostCommon() efficiently?

Why not use a SortedMap internally?

private final Map c = new TreeMap(DESC_FREQ_COMPARATOR);

this way c.entrySet() will return a set of which the iterator will return the keys in order. Although this Set isn't actually a SortedSet it probably fulfills what you need anyway.

General remarks :

Counter

• avoid abbreviations as variable names.

CounterTest

• write seperate tests per assertion : e.g. equality() can be split into 7 different tests. You'll find that you won't need the String argument in your assertions any more.

• do performance tests in a different class than your unit tests. Unit tests should be as lightweight as possible.

Finally consider how I would approach this (JDK8)

EDIT : updated this code to use LongAdder instead of AtomicInteger, as LongAdder performs even better than AtomicInteger for this case.

```
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.LongAdder;
import java.uti

StackExchange Code Review Q#44772, answer score: 17