13
\$\begingroup\$

I would like to give an Integer to whatever demands it. For that I use the following code

public class IntIDGenerator {

    private static int sequence = 1;

    public synchronized static int generate(){
        sequence++;
        return sequence;
    }

}

It is thread-safe because the synchronized term in the method generate(), but I have the feeling that it is possible to implement this generation in a better way. I was thinking in AtomicInteger, but I will have, more or less, the same problem. Any hint?

\$\endgroup\$
  • 6
    \$\begingroup\$ What do you perceive to be a problem with this implementation, and how is using AtomicInteger susceptible to that same problem? \$\endgroup\$ – bowmore Jun 18 '14 at 15:40
6
\$\begingroup\$

And what's wrong with :

public final class SDGIntIDGenerator {

    private static final AtomicInteger sequence = new AtomicInteger(1);

    private SDGIntIDGenerator() {}

    public static int generate(){
        return sequence.getAndIncrement();
    }

}

Atomic Integer is thread safe.
Put private constructor so no instances can be made.
Only static methods means helper class => make class final.

Edit :

While @bowmore is correct you could want more generators for each class, you could do the following :

public final class SDGIntIDGenerator {

    private static final ConcurrentHashMap<Class,AtomicInteger> mapper = new ConcurrentHashMap<Class,AtomicInteger>();

    private SDGIntIDGenerator () {}

    public static int generateId (Class _class) {
        mapper.putIfAbsent(_class, new AtomicInteger(1));
        return mapper.get(_class).getAndIncrement();
    }
}

With this implementation you could ask an id for each class.
This is because the ConcurrentHashMap is also thread safe for some methods.

Important note : Your generator will reset himself after server restart so usage for generating an id for storing in database is a bad idea.

You could also just return the AtomicInteger.
It doesn't matter if you are using a lot of threads and every thread knows that instance of the AtomicInteger.
The AtomicInteger will stay threadsafe.

Footnote : Wrote in java 6, with higher java you could refactor the instantiation of the ConcurrentHashMap.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ There is no reason to make this a static method. Why can't you have multiple instances, each generating a separate series, for different purposes? \$\endgroup\$ – bowmore Jun 18 '14 at 18:30
  • \$\begingroup\$ @bowmore cause the OP did make the method static, the OP should have his reasons for that. \$\endgroup\$ – chillworld Jun 18 '14 at 18:31
  • \$\begingroup\$ Sure, but that should be part of a review remark. Make it non static, extract interface, allow multiple implementations. \$\endgroup\$ – bowmore Jun 18 '14 at 18:33
  • 3
    \$\begingroup\$ @bowmore your Always welcome to write your own review, more answers is more insights. The OP can only be well with that. \$\endgroup\$ – chillworld Jun 18 '14 at 19:36
  • \$\begingroup\$ Returning the AtomicInteger will break encapsulation. Clients could get an id without incrementing, or they could even decrement or increment more than once. \$\endgroup\$ – bowmore Jun 19 '14 at 6:25

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