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 am using a singleton in my DAO. Would I be better with static methods? Also, should I be worried about synchronization of the singleton and my data statures?

public class RAMUserDAO implements GenericDAO<User, String, Boolean>
    {
        private static RAMUserDAO   userDAO = null;
        private Map<String, User>   userList;
        private Map<String, User>   banList;
        private static Logger       log     = LOG.PRODUCTION;

        private RAMUserDAO()
            {
                userList = new ConcurrentHashMap<String, User>();
                banList = new ConcurrentHashMap<String, User>();
            }

        public static RAMUserDAO getRAMUserDAO()
            {
                if (userDAO == null)
                    {
                        synchronized (RAMUserDAO.class)
                            {
                                if (userDAO == null)
                                    {
                                        userDAO = new RAMUserDAO();
                                    }
                            }
                    }
                return userDAO;
            }
    }
share|improve this question

2 Answers 2

up vote 2 down vote accepted

Double-checked locking is broken and must be avoided. The article explains the reasons in depth and provides several alternatives. In this case you can follow the advice in the "Making it work for static singletons" section by immediately initializing the static field.

public class RAMUserDAO implements GenericDAO<User, String, Boolean>
{
    private static RAMUserDAO userDAO = new RAMUserDAO();
    ...

    public static RAMUserDAO getRAMUserDAO() {
        return userDAO;
    }
}

The Java memory model guarantees that the userDAO reference will not be written until all writes have been flushed. The field will be initialized when the class is loaded and safely available to all threads. This has the added benefit of avoiding synchronization to access the DAO.

share|improve this answer
    
I read the website and I think I'm implementing the solution for JDK 5 and above at the bottom. How am I not implementing what they have? Is the helper object different then getRAMUserDAO()? –  user3590149 Aug 12 at 1:01
    
So what you saying is that I would get synchronization without overhead with static? –  user3590149 Aug 12 at 1:08
    
@user3590149 You have to make the field volatile for that to work in JDK 5+, but it won't work in earlier JDKs (probably not an issue). For such a simple class I see no reason to pay the synchronization penalty when you can simply initialize the field directly. –  David Harkness Aug 12 at 1:08
    
I think I been leaning you way. I think I adjust if I find problems down the road. –  user3590149 Aug 12 at 1:09
    
@user3590149 The JVM's class loader synchronizes the loading of each class so you'd pay it only once when the class is first loaded. Your method doesn't need to be synchronized at all. –  David Harkness Aug 12 at 1:09

Your spacing is all over the place, and the double null check against userDAO seems redundant.

I'm not making a comment on the big picture on whether or not the singleton is necessary, but I'd certainly fix getRAMUserDAO() up to look more like this:

public static RAMUserDAO getRAMUserDAO() {
    synchronized (RAMUserDAO.class) {
        if (userDAO == null) {
            userDAO = new RAMUserDAO();
        }
        return userDAO;
    }
}

The Egyptian-style braces are just my general preference. I don't know if there's a Java standard for how to do the braces.

share|improve this answer
    
I'm trying to implement Double Checked Locking for multithreading. I'm not sure if I will need it though. –  user3590149 Aug 12 at 0:42
    
Yes, that's the correct Java brace style, a variation of K&R where the opening brace for classes also goes on the same line. I've never heard of any style called Egyptian, though. :) –  David Harkness Aug 12 at 0:46
    
Same-line opening braces (as in my answer) is known as Egyptian style. blog.codinghorror.com/content/images/uploads/2012/07/… –  nhgrif Aug 12 at 0:48
1  
You shouldn't change the code in your answer. I was commenting on the most-likely preferred formatting style of someone who would maintain this code. –  nhgrif Aug 12 at 0:52
1  
@user3590149 When writing code for your own projects, pick a style that you like and use it consistently. The only issue with it is that it quickly leads to line-wrapping (and horizontal scroll bars here), but that's less important than consistency. –  David Harkness Aug 12 at 0:52

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.