Listing Books nicely - a tour through a webapp

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: koray@tugay.biz 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

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: koray@tugay.biz 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.

StackExchange Code Review Q#100529, answer score: 3