Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a HashMap<Token, Integer>, which counts occurrences of Token. Each time Token is found, value in map should be incremented. I do this this way:

        Map<Token, Integer> occurrences = new HashMap<Token, Integer>();
        // ...

        public void tokenFound(Token token) {

            Integer numberOfOccurs = occurrences.get(token);
            Integer newNumberOfOccurs = new Integer((numberOfOccurs == null) ? 1 : numberOfOccurs.intValue() + 1);
            occurrences.put(token, newNumberOfOccurs);
        }

Is there a more elegant way to do this?

share|improve this question
    
Functors... if your weren't Java bound... –  awashburn Jul 15 at 20:01

3 Answers 3

up vote 11 down vote accepted

You have several different options for this:

Guava

Google's Guava Library introduces the idea of a Multiset which is capable of counting the occurrences, and also provides a couple of other features.

Java 8

If you are using Java 8 (which I highly recommend if you have the ability to do so), your tokenFound method can simply be this:

occurrences.merge(token, 1, (oldValue, one) -> oldValue + one);

Or this:

occurrences.compute(token, (tokenKey, oldValue) -> oldValue == null ? 1 : oldValue + 1);

Note that as of Java 7, you can initialize the map with the "diamond operator":

Map<Token, Integer> occurrences = new HashMap<>();

Without Java 8, no libraries

If you are unable to use Java 8 and don't want to add Guava as a third party library to your project, there are a small part you can do to simplify your existing code:

Integer previousValue = occurrences.get(token);
occurrences.put(token, previousValue == null ? 1 : previousValue + 1);

More specifically:

  • Using the new Integer constructor is not necessary, Java automatically uses boxing to do this. For Integer values close to zero, this will actually save you a little bit because Java keeps some integers cached.
  • You don't need the newNumberOfOccurs variable as it's only used once.
share|improve this answer
    
To supplement the integer caching part: docs.oracle.com/javase/7/docs/api/java/lang/… (-128 to 127) –  h.j.k. Jul 15 at 14:07

Guava's Multiset and its AtomicLongMap are designed for this kind of counting.

See also:

  • Guava's new collection types, explained.
  • Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)
share|improve this answer

I feel the non library answers can be improved, so here's my take at those.

For java 7 :

private final Map<Token, AtomicInteger> occurrences = new HashMap<>();

public void tokenFound(Token token) {
    if (!occurrences.containsKey(token)) {
        occurrences.put(token, new AtomicInteger(1));
        return;
    }
    occurrences.get(token).incrementAndGet();
}

You use AtomicInteger as value type, allowing an easy incrementAndGet(), instead of having to overwrite the bucket in the Map.

For Java 8 :

private final Map<Token, LongAdder> occurrences = new HashMap<>();

public void tokenFound(Token token) {
    occurrences.computeIfAbsent(token, (t) -> new LongAdder()).increment();
}

LongAdder is a type specifically made for tallying (especially under heavy concurrency). The added computeIfAbsent() method on Map and the addition of lambdas turn this whole thing into a one-liner.

If you're using java 7, I'd opt for Guava, but if you're on 8 simply use the java.util classes.

share|improve this answer
1  
I would beg you not to use AtomicInteger for this, but only to use it if you're actually running in a concurrent context. If I had to maintain this code and found out that AtomicInteger was being used to avoid a single extra line of code, I'd be pretty annoyed for the time I spent trying to figure out how this class was used concurrently. –  Chris Hayes Jul 16 at 2:56
    
@ChrisHayes you can also simply roll your own wrapper around an int with an increment() method. The 'elegance' from my solution does not come from using AtomicInteger per se, but from using a mutable value type in the map. If you do use AtomicInteger, you can annotate the class with the JCIP annotation @NotThreadSafe to avoid confusion. –  bowmore Jul 16 at 4:45
    
I think I'd be more confused if I saw a class which went out of the way to use thread-safe types within itself and was annotated as not thread safe. ;) I like the elegance of simply being able to call increment, to be sure. –  Chris Hayes Jul 16 at 4:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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