Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I created a DAO class which is base class for all other DAO classes.

I use Spring Framework 4 and Hibernate 4.

Question: Is there anything that could be done better?

public class GenericDaoImpl<E, I extends Serializable> implements GenericDao<E, I> {
    private Class<E> entityClass;

    @Autowired
    private SessionFactory sessionFactory;

    public GenericDaoImpl(Class<E> entityClass) {
        this.entityClass = entityClass;
    }

    @Override
    public Session getCurrentSession() {
        return sessionFactory.getCurrentSession();
    }

    @Override
    @SuppressWarnings("unchecked")
    public List<E> findAll() throws DataAccessException {
        return getCurrentSession().createCriteria(entityClass).list();
    }

    @Override
    @SuppressWarnings("unchecked")
    public E find(I id) {
        return (E) getCurrentSession().get(entityClass, id);
    }

    @Override
    public void create(E e) {
        getCurrentSession().save(e);
    }

    @Override
    public void update(E e)  {
        getCurrentSession().update(e);
    }

    @Override
    public void delete(E e) {
        getCurrentSession().delete(e);
    }

    @Override
    public void flush() {
        getCurrentSession().flush();
    }
}

Sample DAO class which extends base DAO class is for example this one:

@Repository("articleDao")
public class ArticleDaoImpl extends GenericDaoImpl<ArticleEntity, Long> implements ArticleDao {
    public ArticleDaoImpl(){
        super(ArticleEntity.class);
    }
}
share|improve this question
    
I would shorten getCurrentSession to getSession which is clear enough. – David Harkness May 3 '14 at 18:17
    
@DavidHarkness It is the same as original name from sessionFactory. I don't think that it is an improvement. – user41403 May 7 '14 at 18:50

To be able to create a parameter-less constructor add the following code into your constructor. It will use reflection to set entityClass. This way you don't even need to worry about passing in a class type, you can extend out your generic DAO and its type will be set by parameterisation.

public GenericDaoImpl() {
    Type e = getClass().getGenericSuperclass();
    ParameterizedType pt = (ParameterizedType) e;
    entityClass = (Class<E>) pt.getActualTypeArguments()[0];
}

This removes the need for having constructors in subclasses of your GenericDAOImpl.

Why don't you combine your create and update methods? It makes for easier service calls.

@Override
public void saveOrUpdate(E e){
    getCurrentSession().saveOrUpdate(e);
}

Hibernate will automatically make the determination whether a save or update call is appropriate. If your ID field is set, then perform an update, otherwise perform a save.

Also, I would remove getCurrentSession() from your interface and change the method to private. You shouldn't be accessing the Hibernate session from anywhere outside the DAO so there is no reason to expose out your getCurrentSession() method.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.