Necessary singleton in DAO

Question

I am using a singleton in my DAO. Would I be better off with static methods? Also, should I be worried about synchronization of the singleton and my data structures?

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;
            }
    }

Answer 1

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.

Answer 2

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.