2
\$\begingroup\$

Any comments? I tried to make my code modular and testable. I tried to make use of dependency injection without using any frameworks. All the classes are stateless, immutable and thread-safe (I think). I also tried to code to interfaces, not implementations.

Utils

package biz.tugay.sakila.core.utils;

import javax.persistence.EntityManager;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:26 PM
 */
public interface SakilaEntityManagerFactory {
    EntityManager getEntityManager();
    void closeFactory();
}

package biz.tugay.sakila.core.utils;

import javax.persistence.EntityManager;
import javax.persistence.EntityManagerFactory;
import javax.persistence.Persistence;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 6:00 PM
 */
public class SakilaEntityManagerFactoryImpl implements SakilaEntityManagerFactory {

    private static EntityManagerFactory entityManagerFactory;

    public SakilaEntityManagerFactoryImpl(String persistenceUnitName) {
        if (entityManagerFactory == null) {
            entityManagerFactory = Persistence.createEntityManagerFactory(persistenceUnitName);
        }
    }

    public EntityManager getEntityManager() {
        final EntityManager entityManager = entityManagerFactory.createEntityManager();
        return entityManager;
    }

    public void closeFactory() {
        entityManagerFactory.close();
    }
}

Model Layer

package biz.tugay.sakila.core.model;

import java.io.Serializable;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:56 PM
 */
public abstract class BaseModelObject implements Serializable {
    protected int id;
    public abstract int getId();
    public abstract void setId(int id);
}

package biz.tugay.sakila.core.model;

import javax.persistence.*;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:20 PM
 */

@Entity
@Table(name = "actor")
public class Actor extends BaseModelObject {

    private String firstname;
    private String lastname;

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "id")
    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    @Basic
    @Column(name = "first_name")
    public String getFirstname() {
        return firstname;
    }

    public void setFirstname(String firstname) {
        this.firstname = firstname;
    }

    @Basic
    @Column(name = "last_name")
    public String getLastname() {
        return lastname;
    }

    public void setLastname(String lastname) {
        this.lastname = lastname;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Actor actor = (Actor) o;

        if (firstname != null ? !firstname.equals(actor.firstname) : actor.firstname != null) return false;
        return lastname != null ? lastname.equals(actor.lastname) : actor.lastname == null;

    }

    @Override
    public int hashCode() {
        int result = firstname != null ? firstname.hashCode() : 0;
        result = 31 * result + (lastname != null ? lastname.hashCode() : 0);
        return result;
    }
}

DAO Layer package biz.tugay.sakila.core.dao;

import biz.tugay.sakila.core.model.BaseModelObject;

import javax.persistence.EntityManager;
import java.util.Collection;
import java.util.List;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:47 PM
 */
public interface BaseDao<Entity extends BaseModelObject> {
    EntityManager entityManager();
    Entity get(EntityManager entityManager, int id);
    List<Entity> getAll(EntityManager entityManager);
    void beginTransaction(EntityManager entityManager);
    void insertNew(EntityManager entityManager, Entity entity);
    void commit(EntityManager entityManager);
    void rollback(EntityManager entityManager);
    void flush(EntityManager entityManager);
    void close(EntityManager entityManager);
    void insertMultiple(EntityManager entityManager, Collection<Entity> actors);
    void remove(EntityManager entityManager, Entity e);
    void update(EntityManager entityManager, Entity e);
}

package biz.tugay.sakila.core.dao;

import biz.tugay.sakila.core.model.BaseModelObject;
import biz.tugay.sakila.core.utils.SakilaEntityManagerFactory;

import javax.persistence.EntityManager;
import javax.persistence.EntityTransaction;
import javax.persistence.TypedQuery;
import java.util.Collection;
import java.util.List;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:49 PM
 */
public abstract class BaseDaoImpl<Entity extends BaseModelObject> implements BaseDao<Entity> {

    private final SakilaEntityManagerFactory sakilaEntityManagerFactory;
    protected final Class<Entity> clazz;

    public BaseDaoImpl(SakilaEntityManagerFactory sakilaEntityManagerFactory, Class<Entity> clazz) {
        this.sakilaEntityManagerFactory = sakilaEntityManagerFactory;
        this.clazz = clazz;
    }

    @Override
    public EntityManager entityManager() {
        final EntityManager entityManager = sakilaEntityManagerFactory.getEntityManager();
        return entityManager;
    }

    @Override
    public Entity get(EntityManager entityManager, int id) {
        final Entity entity = entityManager.find(clazz, id);
        return entity;
    }

    @Override
    public List<Entity> getAll(EntityManager entityManager) {
        final String modelName = clazz.getSimpleName();
        final String query = String.format("From %s", modelName);
        final TypedQuery<Entity> typedQuery = entityManager.createQuery(query, clazz);
        final List<Entity> resultList = typedQuery.getResultList();
        return resultList;
    }

    public void beginTransaction(EntityManager entityManager) {
        final EntityTransaction transaction = entityManager.getTransaction();
        transaction.begin();
    }

    @Override
    public void insertNew(EntityManager entityManager, Entity entity) {
        entityManager.persist(entity);
    }

    @Override
    public void insertMultiple(EntityManager entityManager, Collection<Entity> entityCollection) {
        for (Entity entity : entityCollection) {
            insertNew(entityManager, entity);
        }
    }

    @Override
    public void update(EntityManager entityManager, Entity entity) {
        entityManager.merge(entity);
    }

    @Override
    public void remove(EntityManager entityManager, Entity entity) {
        final Entity t = get(entityManager, entity.getId());
        entityManager.remove(t);
    }

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

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

    @Override
    public void flush(EntityManager entityManager) {
        entityManager.flush();
    }

    @Override
    public void close(EntityManager entityManager) {
        entityManager.close();
    }
}

package biz.tugay.sakila.core.dao;

import biz.tugay.sakila.core.model.Actor;

import javax.persistence.EntityManager;
import java.util.Collection;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 6:44 PM
 */
public interface ActorDao extends BaseDao<Actor> {
    Collection<Actor> byLastname(EntityManager entityManager, String lastname);
}

package biz.tugay.sakila.core.dao;

import biz.tugay.sakila.core.model.Actor;
import biz.tugay.sakila.core.utils.SakilaEntityManagerFactory;

import javax.persistence.EntityManager;
import javax.persistence.TypedQuery;
import java.util.Collection;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/13/2016
 * Time: 8:23 PM
 */
public class ActorDaoImpl extends BaseDaoImpl<Actor> implements ActorDao {

    public ActorDaoImpl(SakilaEntityManagerFactory sakilaEntityManagerFactory) {
        super(sakilaEntityManagerFactory, Actor.class);
    }

    /* Ignores case! */
    @Override
    public Collection<Actor> byLastname(EntityManager entityManager, String lastname) {
        final String query = "SELECT a FROM Actor a WHERE UPPER(a.lastname) = :lastname";
        final TypedQuery<Actor> byLastnameQuery = entityManager.createQuery(query, clazz);
        byLastnameQuery.setParameter("lastname", lastname.toUpperCase());
        final Collection<Actor> byLastnameResultList = byLastnameQuery.getResultList();
        return byLastnameResultList;
    }
}

Service Layer package biz.tugay.sakila.core.service;

import biz.tugay.sakila.core.model.BaseModelObject;

import java.util.List;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 4:42 PM
 */
public interface BaseService<Entity extends BaseModelObject> {
    Entity get(int id);
    List<Entity> getAll();
    void insertNew(Entity entity);
    void delete(Entity entity);
    void update(Entity entity);
}

package biz.tugay.sakila.core.service;

import biz.tugay.sakila.core.dao.BaseDao;
import biz.tugay.sakila.core.model.BaseModelObject;

import javax.persistence.EntityManager;
import java.util.List;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 4:30 PM
 */
public abstract class BaseServiceImpl<Dao extends BaseDao, Entity extends BaseModelObject> implements BaseService<Entity> {

    final Dao dao;

    public BaseServiceImpl(Dao dao) {
        this.dao = dao;
    }

    public Entity get(int id) {
        final EntityManager entityManager = dao.entityManager();
        final BaseModelObject object = dao.get(entityManager, id);
        dao.close(entityManager);
        return (Entity) object;
    }

    public final List<Entity> getAll() {
        final EntityManager entityManager = dao.entityManager();
        final List<Entity> all = dao.getAll(entityManager);
        dao.close(entityManager);
        return all;
    }

    public final void insertNew(final Entity entity) {
        final EntityManager entityManager = dao.entityManager();
        dao.insertNew(entityManager, entity);
        dao.beginTransaction(entityManager);
        dao.commit(entityManager);
        dao.close(entityManager);
    }

    public final void update(final Entity entity) {
        final EntityManager entityManager = dao.entityManager();
        dao.beginTransaction(entityManager);
        dao.update(entityManager, entity);
        dao.commit(entityManager);
        dao.close(entityManager);
    }

    public final void delete(final Entity entity) {
        final EntityManager entityManager = dao.entityManager();
        dao.beginTransaction(entityManager);
        dao.remove(entityManager, entity);
        dao.commit(entityManager);
        dao.close(entityManager);
    }
}

package biz.tugay.sakila.core.service;

import biz.tugay.sakila.core.model.Actor;

import java.util.Collection;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 6:38 PM
 */
public interface ActorService extends BaseService<Actor> {
    Collection<Actor> byLastname(String lastname);
}

package biz.tugay.sakila.core.service;

import biz.tugay.sakila.core.dao.ActorDao;
import biz.tugay.sakila.core.model.Actor;

import javax.persistence.EntityManager;
import java.util.Collection;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 4:04 PM
 */
public final class ActorServiceImpl extends BaseServiceImpl<ActorDao, Actor> implements ActorService {

    public ActorServiceImpl(ActorDao actorDAO) {
        super(actorDAO);
    }

    @Override
    public final Collection<Actor> byLastname(final String lastname) {
        final EntityManager entityManager = dao.entityManager();
        final Collection<Actor> byLastname = dao.byLastname(entityManager, lastname);
        dao.close(entityManager);
        return byLastname;
    }
}

Web Layer(JSF Managed Beans)

package biz.tugay.sakila.web;

import javax.faces.context.FacesContext;
import javax.servlet.ServletContext;
import javax.servlet.http.HttpServletRequest;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 9:52 PM
 */
public class BasePageBacking {

    protected <BeanClass> BeanClass getBean(String beanName, Class<BeanClass> clazz) {
        final ServletContext servletContext
                = (ServletContext) FacesContext.getCurrentInstance().getExternalContext().getContext();
        final Object bean = servletContext.getAttribute(beanName);
        final BeanClass castBean = clazz.cast(bean);
        return castBean;
    }

    protected HttpServletRequest getRequest() {
        return (HttpServletRequest) FacesContext.getCurrentInstance().getExternalContext().getRequest();
    }
}

package biz.tugay.sakila.web.actor;

import biz.tugay.sakila.core.model.Actor;
import biz.tugay.sakila.core.service.ActorService;
import biz.tugay.sakila.web.BaseFormBacking;
import lombok.Getter;
import lombok.Setter;

import javax.annotation.PostConstruct;
import javax.faces.bean.ManagedBean;
import javax.faces.bean.RequestScoped;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/15/2016
 * Time: 4:29 PM
 */

@ManagedBean
@RequestScoped
public final class ActorFormBacking extends BaseFormBacking {

    @Getter
    @Setter
    private Actor actor;

    private final ActorService actorService;

    @SuppressWarnings("unused")
    // Invoked by JSF!
    public ActorFormBacking() {
        actorService = getBean("actorService", ActorService.class);
        actor = new Actor();
    }

    @PostConstruct
    // Invoked by JSF!
    public final void init() {
        final String actorId = getRequest().getParameter("actorId");
        if (actorId != null) {
            id = Integer.parseInt(actorId);
            actor = actorService.get(id);
        }
    }

    public final String insertNewActor() {
        actorService.insertNew(actor);
        return "actorInserted";
    }

    public final String updateActor(final int id) {
        actor.setId(id);
        actorService.update(actor);
        return "actorUpdated";
    }

    public final String deleteActor(Actor actor) {
        actorService.delete(actor);
        return "actorDeleted";
    }

    // For unit tests
    public ActorFormBacking(ActorService actorService) {
        this.actorService = actorService;
    }
}

Initializing the application when the server starts:

package biz.tugay.sakila.web.listener;

import biz.tugay.sakila.core.dao.ActorDao;
import biz.tugay.sakila.core.dao.ActorDaoImpl;
import biz.tugay.sakila.core.service.ActorService;
import biz.tugay.sakila.core.service.ActorServiceImpl;
import biz.tugay.sakila.core.utils.SakilaEntityManagerFactory;
import biz.tugay.sakila.core.utils.SakilaEntityManagerFactoryImpl;

import javax.servlet.ServletContext;
import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;
import javax.servlet.annotation.WebListener;

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 5:31 PM
 */

@WebListener
public class SakilaApplicationServletContextListenerImpl implements ServletContextListener {

    @Override
    public void contextInitialized(ServletContextEvent sce) {
        final ServletContext servletContext = sce.getServletContext();

        final SakilaEntityManagerFactory sakilaEntityManagerFactory = new SakilaEntityManagerFactoryImpl("SakilaPU");
        servletContext.setAttribute("sakilaEntityManagerFactory", sakilaEntityManagerFactory);

        final ActorDao actorDao = new ActorDaoImpl(sakilaEntityManagerFactory);
        servletContext.setAttribute("actorDao", actorDao);

        final ActorService actorService = new ActorServiceImpl(actorDao);
        servletContext.setAttribute("actorService", actorService);
    }

    @Override
    public void contextDestroyed(ServletContextEvent sce) {
        final ServletContext servletContext = sce.getServletContext();
        final SakilaEntityManagerFactory sakilaEntityManagerFactory = (SakilaEntityManagerFactory) servletContext.getAttribute("sakilaEntityManagerFactory");
        sakilaEntityManagerFactory.closeFactory();
    }
}

The source code is also available on GitHub.

\$\endgroup\$
1
  • \$\begingroup\$ I don't know why people up-voted this question, because it does not provide any description of what the problem is / what the code is trying to achieve. \$\endgroup\$ Sep 29 '16 at 11:15
2
+250
\$\begingroup\$
  1. SakilaEntityManagerFactory can extend AutoClosable instead of providing it's own closeFactory() method. By extending AutoClosable you can take advantage of sopport from IDEs and various static code analysis tools that will report leaked(unclosed) factories.

  2. Your model does not really need to be Serializable as you do not use the native java serialization. Libraries like Gson,Jackson, etc do not rely on native java serialization and do not require objects to implement Serializable

  3. Not a big issue, but seems "unnatural" to me. On every project I've worked, the base class names follow the pattern <Name>Base, but you are doing it in the opposite way Base<Name>. Example: BaseModelObject should be ModelObjectBase

  4. Do you really need DAOs ? If you take a look at your code you'll see that your daos just delegate directly to the EntityManager. Then instead of "dao.newInstance()" why don't you directly call entityManager.persist() ? DAOs can be used to hide the underlying implementation, but your DAOs leak that they are based on EntityManager. For simple CRUD uperations DAOs are unnecessary and just complicate the code.

  5. Your DAO methods are misleading. insertNew() does not necessarily create a new entry at the database. If the passed entity is already a "managed object" insertNew() won't do anything! If the entity is not a managed object, but has non null @ID, then calling persist() (or actually on flush/transaction end) will throw PersistenceException if ther eis already an entry with that ID. the method update() suffers from simmilar issues. For more details read about persist()/merge() and how they work and what they do.

  6. Instead of using some magic strings for attributes like "sakilaEntityManagerFactory" you can use the interface/class names: SakilaEntityManagerFactory.class.getName()

  7. Either design for inheritance or explicitly forbid it. If it's (obviously) not an interface or an abstract/base class you should consider marking it as final. An exception can be the model objects, because some persistence libraries may not be able to instrument them if they are final.

  8. The SakilaApplicationServletContextListenerImpl name is just terrible. Consider shorter and more descriptive name like "PersistenceServiceInitializer" or "PersistenceServiceLifecycle(Listener)", etc

\$\endgroup\$
1
\$\begingroup\$

Consider using proper Javadoc

Instead of writing

/**
 * User: Koray Tugay (koray@tugay.biz)
 * Date: 9/14/2016
 * Time: 5:31 PM
 */

more canonical usage would be (for example):

/**
 * (Here, describe your class/interface.)
 *
 * @author Koray Tugay
 * @version 9/14/2016 5:31 PM
 */

Using minor utilities

In the Actor.java, you can rewrite a couple of methods more succintly (your code commented):

//    @Override
//    public boolean equals(Object o) {
//        if (this == o) return true;
//        if (o == null || getClass() != o.getClass()) return false;
//
//        Actor actor = (Actor) o;
//
//        if (firstname != null ? !firstname.equals(actor.firstname) : actor.firstname != null) return false;
//        return lastname != null ? lastname.equals(actor.lastname) : actor.lastname == null;
//
//    }

public boolean equals(Object o) {
    if (o == null || !getClass().equals(o.getClass())) {
        return false;
    }

    Actor other = (Actor) o;
    return Objects.equals(firstname, other.firstname) &&
           Objects.equals(lastname, other.lastname);
}

//    @Override
//    public int hashCode() {
//        int result = firstname != null ? firstname.hashCode() : 0;
//        result = 31 * result + (lastname != null ? lastname.hashCode() : 0);
//        return result;
//    }

@Override
public int hashCode() {
    int result = Objects.hashCode(firstname);
    return 31 * result + Objects.hashCode(lastname);
}

//    @Override
//    public String toString() {
//        return "Actor{" +
//                "firstname='" + firstname + '\'' +
//                ", lastname='" + lastname + '\'' +
//                '}';
//    }

@Override
public String toString() {
    return "Actor{firstname='" + firstname +
               "', lastname='" + lastname + "'}";
}

Otherwise, your code looks pretty good.

Hope that helps.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.