Listing Books nicely - a tour through a webapp

Question

In this exercise, I do not want to use any container such as Spring or Glassfish. I am deploying my application to Tomcat. I only use JPA. What I want to achieve is to follow best practices and OOP concepts correctly, and a good separation of concerns and layering.

I have a database table like this:

mysql> DESCRIBE book;
+-------------+--------------+------+-----+---------+
| Field       | Type         | Null | Key | Default |
+-------------+--------------+------+-----+---------+
| isbn        | varchar(13)  | NO   | PRI | NULL    |
| name        | varchar(64)  | NO   | UNI | NULL    |
| publishDate | date         | YES  |     | NULL    |
| price       | decimal(8,2) | YES  |     | NULL    |
| publisher   | varchar(6)   | YES  | MUL | NULL    |
+-------------+--------------+------+-----+---------+

and here is my Entity:

@Entity
@Table(name = "book")
public class Book {

    @Id
    private String isbn;

    @Basic
    private String name;

    @Basic
    private Date publishDate;

    @Basic
    private double price;

    @Basic
    private String publisher;

    // Getters, Setters...

This is my PersistenceUtil class:

package biz.tugay.books10Aug.dao;
/* User: [email protected] Date: 10/08/15 Time: 22:22 */

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

public class PersistenceUtil {

    private static EntityManagerFactory entityManagerFactory;

    public static void initalizeEntityManagerFactory() {
        if (entityManagerFactory == null || !entityManagerFactory.isOpen()) {
            entityManagerFactory = Persistence.createEntityManagerFactory("bookshop");
        }
    }

    public static EntityManagerFactory getEntityManagerFactory() {
        return entityManagerFactory;
    }

    public static EntityManager getEntityManager() {
        if (entityManagerFactory == null || !entityManagerFactory.isOpen()) {
            initalizeEntityManagerFactory();
        }
        EntityManager entityManager = entityManagerFactory.createEntityManager();
        return entityManager;
    }

    public static void closeEntityManagerFactory() {
        entityManagerFactory.close();
    }

}

and in my web application I have a ContextListener:

package biz.tugay.books10Aug.web;
/* User: [email protected] Date: 10/08/15 Time: 22:24 */

import biz.tugay.books10Aug.dao.PersistenceUtil;

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

@WebListener
public class BookShopServletContextListener implements ServletContextListener {

    @Override
    public void contextInitialized(ServletContextEvent servletContextEvent) {
        PersistenceUtil.initalizeEntityManagerFactory();
    }

    @Override
    public void contextDestroyed(ServletContextEvent servletContextEvent) {
        PersistenceUtil.closeEntityManagerFactory();
    }

}

#1 Is this the right way to initialise Persistence Context? Or should this be responsibility of DAO / Service classes?

My DAO Layer:

package biz.tugay.books10Aug.dao;
/* User: [email protected] Date: 10/08/15 Time: 22:50 */

import biz.tugay.books10Aug.model.Book;

public interface BookDao {
    public void persist(Book book);
    public Book getWithIsbn(String isbn);
}

and the implementation:

package biz.tugay.books10Aug.dao;
/* User: [email protected] Date: 10/08/15 Time: 22:54 */

import biz.tugay.books10Aug.model.Book;

import javax.persistence.EntityManager;

public class BookDaoImpl implements BookDao {

    private EntityManager entityManager;

    public BookDaoImpl(EntityManager entityManager) {
        this.entityManager = entityManager;
    }

    @Override
    public void persist(Book book) {
        entityManager.persist(book);
    }

    @Override
    public Book getWithIsbn(String isbn) {
        Book book = entityManager.find(Book.class, isbn);
        return book;
    }

}

#2 Is it ok that the DAO Layer requires an EntityManager and assumes that its transaction has already begun? Should it be the DAOs responsibility to obtain the EntityManager and/or begin/commit transaction?

Here is my Service Layer:

package biz.tugay.books10Aug.service;
/* User: [email protected] Date: 11/08/15 Time: 00:12 */

import biz.tugay.books10Aug.model.Book;

import java.util.Date;

public interface BookService {

    public void createBook(String isbn, String name, Date publishDate, double price, String publisher);

    public Book getWithIsbn(String isbn);

}

and the implementation:

package biz.tugay.books10Aug.service;
/* User: [email protected] Date: 11/08/15 Time: 00:12 */

import biz.tugay.books10Aug.dao.BookDao;
import biz.tugay.books10Aug.dao.BookDaoImpl;
import biz.tugay.books10Aug.dao.PersistenceUtil;
import biz.tugay.books10Aug.model.Book;

import javax.persistence.EntityManager;
import javax.persistence.EntityTransaction;
import java.util.Date;

public class BookServiceImpl implements BookService {

    private BookDao bookDao;
    private EntityManager entityManager;

    public BookServiceImpl() {
        EntityManager entityManager = PersistenceUtil.getEntityManager();
        bookDao = new BookDaoImpl(entityManager);
        this.entityManager = entityManager;
    }

    @Override
    public void createBook(String isbn, String name, Date publishDate, double price, String publisher) {
        Book book = new Book();
        book.setIsbn(isbn);
        book.setName(name);
        book.setPublishDate(publishDate);
        book.setPrice(price);
        book.setPublisher(publisher);
        EntityTransaction transaction = entityManager.getTransaction();
        transaction.begin();
        bookDao.persist(book);
        transaction.commit();
    }

    @Override
    public Book getWithIsbn(String isbn) {
        EntityTransaction transaction = entityManager.getTransaction();
        transaction.begin();
        Book withIsbn = bookDao.getWithIsbn(isbn);
        transaction.commit();
        return withIsbn;
    }

}

#3 Again, I have the same questions on my mind. Is this class a mess like this? It does not have any dependencies, it obtains its EntityManager from the PersistenceUtil class in the Constructor. Is this ok? How about the entityManager field and starting / committing transactions which surrounds DAO calls?

And finally I have a view like this:

<form action="<c:url value="/book"/>" method="post">
    <label for="isbn">
        isbn:
    </label>
    <input type="text" name="isbn" id="isbn"/>
    <input type="submit"/>
</form>

and a Servlet:

@WebServlet(urlPatterns = "/book")
public class BookServlet extends HttpServlet {

    @Override
    protected void doPost(HttpServletRequest req, HttpServletResponse resp)
            throws ServletException, IOException {
        BookService bookService = new BookServiceImpl();
        String isbn = req.getParameter("isbn");
        Book withIsbn = bookService.getWithIsbn(isbn);
        req.setAttribute("book", withIsbn);
        req.getRequestDispatcher("/isbn.jsp").forward(req,resp);
    }
}

#4 Is it ok that the Servlet initialises a Service object for each call? Or should it have a private static Service object?(I think not, but I am not sure thus asking..)

As I said, (just for learning purposes) I want to make such a basic web app which is maintainable and which follows OOP principles, without using any other frameworks such as Spring.

Answer 1

Doing work in constructor

This

public BookServiceImpl() {
    EntityManager entityManager = PersistenceUtil.getEntityManager();
    bookDao = new BookDaoImpl(entityManager);
    this.entityManager = entityManager;
}

should be

public BookServiceImpl(EntityManager entityManager, BookDao bookDao) {
    this.entityManager = entityManager;
    this.bookDao = bookDao;
}

move the code to the calling site. Then extract the moved code to a factory method, as you will use it multiple times. Voila, no mess in constructor, no unnecessary dependency to an evil singleton.

// call site
BookService bookService = getBookService();
bookService.getWithIsbn(...)

// factory method
public BookService getBookService() {
     EntityManager entityManager = PersistenceUtil.getEntityManager();
     BookDao bookDao = new BookDaoImpl(entityManager);
     return new BookServiceImpl(entityManager, bookDao);
}

This method would be in the servlet at first. You can move this to a factory class, so that you can call it in multiple servlets using this service.

public class BookServiceFactory {

    public BookService create() {
       // same method just cut and paste.
    }
}

So that getBookService would now read;

// factory method
public BookService getBookService() {
     return new BookServiceFactory().create();
}

Transaction Management

Transaction should be rollback in case of an exception.

EntityManager should be closed in order for entities to be detached. (As far as I can remember. I don't do entity life-cycle management by hand, so consult the documentation for details.)

Noise

Use a version control system, such as git, if you aren't already; and remove these unnecessary comments.

/* User: [email protected] Date: 10/08/15 Time: 22:22 */

Is it ok that the Servlet initialises a Service object for each call?

Depends. Try it both ways and see if memory consumption and response times change.

However since there is a choice here, this means extract the code to a method. So that when you need to change your decision, you will change the code in one place.

The end result should look like this:

    BookService bookService = getBookService();

Or should it have a private static Service object?(I think not, but I am not sure thus asking..)

Avoid static as much as possible. They introduce unnecessary dependencies. And cause hard to debug memory leaks.

You can change it to use the singleton scope like this:

public BookService getBookService() {
     return (BookService)getServletContext().getAttribute("books10Aug.BookService");
}

You should set the attribute during servlet context initialization. There would then be one BookService per (deployed) application.