Best Practices for DAO Implementation

Question

I have been using a DAO pattern to provide access to my persistence tier in an application that I have been building.

One of the things that I have implemented is a 'wrapper' around my DAO implementations for purposes of validation. The wrapper takes in an instance of my DAO as a constructor parameter, and implements a similar interface as the DAO, with the exception being the types of exceptions thrown.

For example:

Business Logic Interface

public interface UserBLInt {

   private void assignRightToUser(int userId, int rightId) throws SomeAppException;

}

DAO Interface

public interface UserDAOInt {

   private void assignRightToUser(int userId, int rightId) throws SomeJPAExcption;

}

Business Logic Implementation

public class UserBLImpl implements  UserBLInt {

   private UserDAOInt userDAO;

   public UserBLImpl(UserDAOInt userDAO){
      this.userDAO = userDAO;
   }

   @Override
   private void assignRightToUser(int userId, int rightId) throws SomeAppException{
      if(!userExists(userId){
         //throw an exception
      }
      try{
         userDAO.assignRightToUser(userId, rightId);
      } catch(SomeJpAException e){
        throw new SomeAppException(some message);
      }
   } 

}

DAO Implementation

public class UserDAOImpl implements UserDAOInt {
      //......
      @Override
      public void assignRightToUser(int userId, int rightId){
         em.getTransaction().begin();
         User userToAssignRightTo = em.find(User.class, userId);
         userToAssignRightTo.getRights().add(em.find(Right.class, rightId));
         em.getTransaction().commit();
      }
}

This is just a quick example, but my question is, it seems 'redundant' within the DAO implementation to do another check to be sure that the User isn't null before adding a Right, but, as a programmer, I see an opportunity for a null pointer.

Obviously I could add a null check after calling find on the entity manager and throw an exception if null is returned, but that's the whole purpose of having the DAO wrapped in a business logic implementation, to do all the validation work beforehand, so that the DAO code is clean and doesn't have to do much in the way of null checks or much logic at all. Since I have the wrapper around the DAO, is it still a good idea to do the null checking anyway in the DAO? I know in theory the object could be deleted between the business logic call and the dao call, it's just unlikely, and checking for null seems like duplicated work. What is the best practice for a situation like this?

EDIT:

Does this look like a suitable DAO modification?

public EntityManager beginTransaction(){
    EntityManager entityManager = entityManagerFactory.createEntityManager();
    EntityTransaction entityTransaction = entityManager.getTransaction();
    entityTransaction.begin();
    return entityManager;
}

public void rollback(EntityManager entityManager){
    entityManager.getTransaction().rollback();
    entityManager.close();
}

public void commit(EntityManager entityManager){
    entityManager.getTransaction().commit();
    entityManager.close();
}

Answer 1

DAOs, although a bit of a now generic and overused term, are (usually) meant to abtract the Data layer (so among other benefits, it could be changed without having to touch the rest of the app).

It seems, though, your DAO is actually doing a lot more than just abstracting the data layer. In:

public class UserDAOImpl implements UserDAOInt {
  ......
  @Override
  public void assignRightToUser(int userId, int rightId){
     em.getTransaction().begin();
     User userToAssignRightTo = em.find(User.class, userId);
     userToAssignRightTo.getRights().add(em.find(Right.class, rightId));
     em.getTransaction().commit();
  }
}

Your DAO knows business logic. It knows that assigning a right to auser is adding a right to the list of rights. (It may seem obvious that assigning a right is just adding it to a list, but imagine this could get, in the future, much more complicated, with side-effects to other users and rights and so on.)

So this assigning does not belong in the DAO. It should be in the business layer. Your DAO should have just something like userDAO.save(user) that the business layer would call once it is done setting the rights and stuff.

Another problem: Your transaction is too localized. It is almost not a transaction.

Remember the transaction is a business unit, something you do atomic (a "batch" of) business work within, not just something you open because the EntityManager makes you.

What I mean, in terms of code, is that the business layer should have the initiative of opening the transaction, not the DAO (actually, the DAO should have "open a transaction" as a service -- method -- that would be called).

Consider, then, opening the transaction in the Business Layer:

public class UserBLImpl implements  UserBLInt {
   ...
   @Override
   private void assignRightToUser(int userId, int rightId) throws SomeAppException{
      userDAO.beginTransaction(); // or .beginUnitOfWork(), if you wanna be fancy

      if(!userExists(userId){
         //throw an exception
         // DON'T FORGET TO ROLLBACK!!! before throwing the exception
      }
      try{
         userDAO.assignRightToUser(userId, rightId);
      } catch(SomeJpAException e){
        throw new SomeAppException(some message);
      }

      userDAO.commit();
   } 
}

Now, to your question: that risk of the DB changing between the userExists() if and the userDAO persisting it still exists... but you have options:

(1) Lock the user until the transaction is over; or (2) Let it be.

1: If the risk of the user being messed up with between those two commands is high (say your system has a lot of concurrent users) and if that problem happened it would be big deal, then consider locking the user for the whole transaction; that is, if I begin working with it, no other transaction may change it.

• Another (better) solution to that if your system has tons os concurrent users is to "design the problem away", that is, rework your design so that what is changed in a business transaction has a more strict (smaller) scope -- the scope of your example is small enough, so this advice may not make so much sense, but just consider your business transaction did a whole lot more (then making it do a whole lot less, each in its turn, could be the solution). This is a whole topic all by itself, so I won't go into more details here, just keep your mind open to that.

2: Another possibility, and you will figure this is the most common approach, if you are dealing with a SQL DB that has constraint checks, such as UNIQUE, is just to let the DAO exception blow away. I mean, it will be such a rare and near-impossible thing to happen that you may as well deal with it by just accepting it can happen and your system will just display a nice message like "Something went wrong, please try again" -- this is just basic costs vs. benefits weighting.

---

Update:

Programatic transaction handling can be tricky (no wonder declarative alternatives, such as Spring and EJB/CDI, are so used). Still, we don't always have the luxury of using them (maybe you are adapting a legacy system, who knows). So here's a suggestion: https://gist.github.com/acdcjunior/94363ea5cdbf4cae41c7

Answer 2

The DAO has too much logic here. The role of a DAO is not to assign rights to a user. That is business logic. The role of a DAO is to find users or find rights. The code that is in the DAO should be in the service:

interface UserDAO {
    User findById(int userId);
    //...
}

interface RightDAO {
    Right findById(int rightId);
    // ...
}

public class UserBLImpl implements  UserBLInt {

    private UserDAO userDAO;
    private RightDAO rightDAO;

    // ...

    @Override
    public void assignRightToUser(int userId, int rightId) {
        User u = userDAO.findById(userId);
        Right right = rightDAO.findById(rightId);
        // do some null checks and throw exception
        user.addRight(right);
    } 
}

This also shows a fundamental problem in your design: the transactions must not be started at the DAO layer. They must be started at the service layer. That's what allows DAO methods to be reusable and services methods to use several DAOs in a single transaction. Not doing so will lead to anemic services that will call DAO methods containing the whole business logic, as your code shows.

And that's why frameworks like EJBs or Spring allow demarcating transactions declaratively: you don't need to explicitely start and commit trannsactions, handle exceptions and rollback exceptions. All you have to do is mark a service method as transactional using an annotation.