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 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?

share|improve this question
5  
What do you perceive to be a problem with this implementation, and how is using AtomicInteger susceptible to that same problem? –  bowmore Jun 18 at 15:40

1 Answer 1

up vote 4 down vote accepted

And what's wrong with :

public final class SDGIntIDGenerator {

    private static final AtomicInteger = 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.

share|improve this answer
2  
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? –  bowmore Jun 18 at 18:30
    
@bowmore cause the OP did make the method static, the OP should have his reasons for that. –  chillworld Jun 18 at 18:31
    
Sure, but that should be part of a review remark. Make it non static, extract interface, allow multiple implementations. –  bowmore Jun 18 at 18:33
2  
@bowmore your Always welcome to write your own review, more answers is more insights. The OP can only be well with that. –  chillworld Jun 18 at 19:36
    
Returning the AtomicInteger will break encapsulation. Clients could get an id without incrementing, or they could even decrement or increment more than once. –  bowmore Jun 19 at 6:25

protected by Malachi Jun 19 at 16:46

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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