Implementing DAO Factory

Question

I'm trying to do three different type of DAOs with 4 subtypes of each. I have RAM, SQL and Serial with each one have four subtypes.

Is a singleton necessary with my implementation within the factory and more specifically with each DAO? Should I use static methods instead? Also I have two factories, is that necessary?

I might be missing the point here of the factory and implementing incorrectly? I only posted RAM and RAMUser with RAM Factory and DAO factory, I figure that's all really need to be post.

//Abstract class DAO Factory
public abstract class DAOFactory
{
    private static Logger   log = LOG.PRODUCTION;

    public abstract GenericDAO<Trade, String, Boolean> getTradeDAO();

    public abstract GenericDAO<User, String, Boolean> getUserDAO();

    public abstract GenericDAO<User, String, Boolean> getLogDAO();

    public abstract GenericDAO<ServiceRequest, String, Boolean> getRequestDAO();

    private static DAOFactoryType   currentUsedStorage;

    /**
     * @return the currentUsedStorage
     */
    public static DAOFactoryType getCurrentUsedStorage()
        {
            return currentUsedStorage;
        }

    /**
     * @param currentUsedStorage
     *            the currentUsedStorage to set
     */
    public void setCurrentUsedStorage(DAOFactoryType currentUsedStorage)
        {
            DAOFactory.currentUsedStorage = currentUsedStorage;
        }

    public static DAOFactory getDAOFactory(DAOFactoryType factoryType)
        {
            switch (factoryType)
            {
                case SQL :
                    currentUsedStorage = DAOFactoryType.SQL;
                    return new SQLDAOFactory();
                case RAM :
                    currentUsedStorage = DAOFactoryType.RAM;
                    return new RAMDAOFactory();
                case SERIAL :
                    currentUsedStorage = DAOFactoryType.SERIAL;
                    return new SerialDAOFactory();

                default :
                    return null;
            }
        }
}

GenericDAO

public interface GenericDAO<U,S,B>
{
    public boolean update(U user, B active) throws NullUserException,
            UserExistException, NullListException, UserInstanceException,
            ReceivedNullParameterException;

    public boolean delete(U user, B active) throws NullUserException,
            UserExistException, NullListException, UserInstanceException,
            ReceivedNullParameterException;

    public U findUserByUserName(S userName, B active)
            throws NullUserException, UserExistException, NullListException,
            UserInstanceException, ReceivedNullParameterException;

    public U findUserByUser(U user, B active)
            throws NullUserException, NullListException, UserInstanceException,
            ReceivedNullParameterException;

    public boolean insert(U user, B active)
            throws ReceivedNullParameterException, UserExistException,
            NullListException, UserInstanceException;
}

RAMDAOFactory

    public class RAMDAOFactory extends DAOFactory
    {
        private static Logger log = LOG.PRODUCTION;

        @Override
        public GenericDAO<Trade, String, Boolean> getTradeDAO()
            {
                return RAMTradeDAO.getRAMTradeDAO();
            }

        @Override
        public GenericDAO<User, String, Boolean> getUserDAO()
            {
                return RAMUserDAO.getRAMUserDAO();
            }

        @Override
        public GenericDAO<User, String, Boolean> getLogDAO()
            {
                return RAMLogDAO.getRAMLogDAO();
            }

        @Override
        public GenericDAO<ServiceRequest, String, Boolean> getRequestDAO()
            {
                return RAMServiceRequestDAO.getRAMRequestDAO();
            }

    }

RAMUserDAO

public class RAMUserDAO implements GenericDAO<User, String, Boolean>
{

    private static RAMUserDAO   userDAO = new RAMUserDAO();
    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()
        {
            return userDAO;
        }

    @Override
    public boolean update(User user, Boolean active) throws NullUserException,
            UserExistException, NullListException, UserInstanceException,
            ReceivedNullParameterException
        {
            // TODO Auto-generated method stub
            return false;
        }

    @Override
    public boolean delete(User user, Boolean active) throws NullUserException,
            UserExistException, NullListException, UserInstanceException,
            ReceivedNullParameterException
        {
            // TODO Auto-generated method stub
            return false;
        }

    @Override
    public User findUserByUserName(String userName, Boolean active)
            throws NullUserException, UserExistException, NullListException,
            UserInstanceException, ReceivedNullParameterException
        {
            // TODO Auto-generated method stub
            return null;
        }

    @Override
    public User findUserByUser(User user, Boolean active)
            throws NullUserException, NullListException, UserInstanceException,
            ReceivedNullParameterException
        {
            // TODO Auto-generated method stub
            return null;
        }

    @Override
    public boolean insert(User user, Boolean active)
            throws ReceivedNullParameterException, UserExistException,
            NullListException, UserInstanceException
        {
            // TODO Auto-generated method stub
            return false;
        }
}

Answer 1

I might be missing the point here of the factory and implementing incorrectly?

Factories are an abstraction for how to obtain instances, as distinct from explicitly pooling, singletonning, or creating instances (through constructors) from client code. Exactly how far into this abstraction you want to go is a matter of debate and, in some cases, ridicule, but this base abstraction is what you need to keep in mind when designing them.

Because factories are a means to an end, it's hard to tell whether your factory will be helpful without seeing your use case. What will your use of the pattern help you accomplish? (As a hunch, I'd recommend you to look into dependency injection.)

Is a singleton necessary with my implementation within the factory and more specifically with each DAO?

No. A singleton is never really necessary anywhere⁽¹⁾ and it's usually a convenience hack to prevent having to pass a reference into a method or store it in context; a bypass of sorts. That's not to say that singletons serve no useful purpose, but that the decision to use a singleton should be an implementation choice, and never a design choice.

_{(1) It can even be downright hazardous in server environments with managed containers.}

The setCurrentUsedStorage is what kind of storage I'm using. So RAM, SQL or Serial implementation. How do you suggest to deal with this?

Ideally, remove the field. If you have a clear and compelling use case that can not be reasonably solved otherwise, keep the field for now, but clearly document the why and the how (and use the HACK pseudo-tag), and consider adding it to your project's to-do list. This really feels like a bug waiting to happen.

It should not be a factory's duty to keep track of what type you are currently interested in. Either the container (configuration) or the client code should know what it wants/needs (and pass it into the factory method): factories are middle men.

---

That's for the abstract part; let's get to concrete suggestions.

Pull singletons up to the factory level

The concern of the DAO implementations is data storage and retrieval. The concern of the factory is how to supply instances. Lifting the singletons from the individual implementations leaves the factory in control of instantiation and life cycle.

Here's a straightforward example of such an approach:

// I like that boilerplate.  That's some nice boilerplate.
public class SimpleStoringFactory extends DAOFactory {
  private final GenericDAO<Trade, String, Boolean> tradeDAO;
  private final GenericDAO<User, String, Boolean> userDAO;
  private final GenericDAO<User, String, Boolean> logDAO;
  private final GenericDAO<ServiceRequest, String, Boolean> requestDAO;

  public SimpleStoringFactory(
      GenericDAO<Trade, String, Boolean> tradeDAO,
      GenericDAO<User, String, Boolean> userDAO,
      GenericDAO<User, String, Boolean> logDAO,
      GenericDAO<ServiceRequest, String, Boolean> requestDAO) {
    this.tradeDAO = tradeDAO;
    this.userDAO = userDAO;
    this.logDAO = logDAO;
    this.requestDAO = requestDAO;
  }

  public GenericDAO<Trade, String, Boolean> getTradeDAO() { return tradeDAO; }
  public GenericDAO<User, String, Boolean> getUserDAO() { return userDAO; }
  public GenericDAO<User, String, Boolean> getLogDAO() { return logDAO; }
  public GenericDAO<ServiceRequest, String, Boolean> getRequestDAO() { return requestDAO; }
}

GenericDAO

public boolean update(U user, B active) throws NullUserException,
        UserExistException, NullListException, UserInstanceException,
        ReceivedNullParameterException;

That's... quite something. Only list exceptions that you expect clients to handle/catch, and see whether built-in, available exception types like IllegalArgumentException suffice.

If this is not feasible, document when exceptions are expected to be thrown, and when the return value is true or false. I could not make this out from the method signature. Compare with:

/**
 * Finds the record identified by <var>user</var> and updates it to the given values.
 * Returns true if changes were made to the record, and false if the record was found,
 * but all current values matched given values (i.e. no update occurred).
 *
 * @return {@code true} iff changes were made to the record.
 * @throws NotFoundException if no record for this user was found.
 */
public boolean update(U user, B active) throws NotFoundException;

Documentation

Auto-generated getter and setter comments are not useful as documentation: they simply repeat the name of the function. Remove these to reclaim some vertical space.

Document what things like active mean in your method parameters. Does it make sense ever not to pass a boolean in there?

Answer 2

The first thing I see is one glaring issue with this code:

• RAM Trade DAO, RAM User DAO, RAM Log DAO, RAM Request DAO
• SQL Trade DAO, SQL User DAO, SQL Log DAO, SQL Request DAO
• Serial Trade DAO, Serial User DAO, Serial Log DAO, Serial Request DAO

In rows, I am seeing repetition of persistence type. In columns, I am seeing repetition of business object type. That means that, given n persistence types and m business object types, there will need to be n*m classes. But these two types are not related! They are dealing with two different levels of abstraction. Your use of GenericDAO<U, S, B> obscures the nature of the different types, making the code more confusing. We should break these apart, and use composition to create the combinations. (Pseudocode follows)

interface DAO {
    insert(<Supertype of all types> user, Boolean active);
    update(<Supertype of all types> user, Boolean active);
    findByUserName(String user, Boolean active);
    findByUser(<Supertype of all types> user, Boolean active);
    delete(<Supertype of all types> user, Boolean active);
}

And classes RAMDAO, SQLDAO, and SerialDAO implement the DAO interface.

interface Repo<T> {
    setDAO(DAO dao);
    insert(T user, Boolean active);
    update(T user, Boolean active);
    findByUserName(String user, Boolean active);
    findByUser(T user, Boolean active);
    delete(T user, Boolean active);
}

And classes TradeRepo, UserRepo, LogRepo, and RequestRepo implement the Repo interface. Notice the setDAO method on Repo. We can inject the DAO we want to use, without depending on what type of persistence the DAO object uses. You can also inject the DAO in the constructor and leave off the setter if you would rather do it that way.

We now have 2 groups of objects, n persistence types, and m business object types, and n+m classes (plus two interfaces). Most importantly, if I add another, say, persistence type, I only need to create one more class, not m more classes.

---

Now we've broken up the coupling between the classes, meaning now we have to address the factory. Here, you are using a concrete factory to create abstract factory objects, based on type of storage. Instead, we can attack it from a different angle. We need to create two different types of object, and compose them together. They have the same lifetime, and one depends on the other, so we can put them together into the same factory.

class RepoFactory {
    RepoFactory(DAOType daoType) {
        this.daoType = daoType;
    }

    getRepoInstance(RepoType repoType) {
        DAO dao;
        switch(daoType) {
            case RAM: dao = new RAMDAO();
            case SQL: dao = new SQLDAO();
            case Serial: dao = new SerialDAO();
        }

        Repo repo;
        switch(repoType) {
            case Trade: repo = new TradeRepo();
            case User: repo = new UserRepo();
            case Log: repo = new LogRepo();
            case Request: repo = new RequestRepo();
        }

        repo.setDAO(dao);
        return repo;
    }
}

RepoFactory is now instantiable, and you can create different Repos with different DAOTypes without having to track a global state. This also eliminates the abstract factories, as our objects can now be created inline. We could also trivially instantiate one or both parts as singletons, independently. Singletons are not necessary for the most part. The only concern here is whether each persistence type allows multiple connections, and what concurrency issues arise out of singleton-vs-non-singleton. Documentation of the underlying implementations and testing are your friends here.

This code is essentially effective at what it is trying to accomplish. We could leave it as it is. One thing I see, though, is that different parts of the getRepoInstance method change at different rates, namely the DAO part and the Repo part. We are probably more likely to add Repos than to add DAOs. Were it me, and I had a few minutes, I would inject a DAOFactory instead of a DAOType enum, and let the factory produce my DAOs. This is optional if you are willing to live with the maintenance overhead, though.

---

You have a lot of generics being tossed around here.

GenericDAO<Trade, String, Boolean>
GenericDAO<User, String, Boolean>
GenericDAO<User, String, Boolean>
GenericDAO<ServiceRequest, String, Boolean>

First, the last two seem to always be String and Boolean. If these are not going to change, you should leave them off, and just use the concrete types in your implementation. Making these variable just adds complexity and noise to the code, for little gain if they are not actually variable.

Second, Trade, User, and ServiceRequest seem like very different things to me. I don't see the rest of your app, so maybe they are more related than they seems, but the names do not indicate this is so. Indeed, the fact that you are passing them as generic arguments supports the idea that they are not related, otherwise you would just use a supertype. If they are similar in some way, you should encapsulate that similarity in an interface that they all implement. You can then use the interface type to handle these different objects in a similar, type-safe way.

If you followed my suggestions above, the generics don't even exist any more, but in the DAO interface, I have a stub type (<Supertype of all types>). This would be the interface type.

---

I have to agree with JvR regarding the excessive number of exceptions that could be thrown. Looking at them, several of them seem to even mean the same thing. Try to wrap them into a single, meaningful exception type for each actual type of error.

---

Like all code, factories should generally strive to be SOLID, but being open/closed can be a little tricky. Here is an article that talks about several different strategies to make factories more open/closed: http://java.globinch.com/patterns/design-patterns/factory-design-patterns-and-open-closed-principle-ocp-in-solid/ It talks about different techniques to make factories more or less abstract, with different trade-offs, and gives code samples to visually show what the code might look like.