Coding style and best practice regarding exception-handling

Question

I need comments on my JPA code. Am I doing it correctly? I need some advice on best practices in Java.

I have this method below:

public Comics add(Comics comics) {
    EntityManager em = EMF.get().createEntityManager();
    EntityTransaction tx = null;
    try{
        tx = em.getTransaction();
        tx.begin();

        em.persist(comics);

        tx.commit();
    }catch(Exception ex){
        ex.printStackTrace();
        if(tx != null && tx.isActive())
            tx.rollback();
    } finally{
        em.close();
    }

    return comics;
}

When I called the method, I just have:

ComicsAccess access = new ComicsAccess();
access.add( comics );

I got no error on the above code, what I want are some suggestions on my coding style. Is the above good code if used in production?

Answer 1

The way you are handling the exception will result in the method returning normally whether or not the method has succeeded. This is bad API design. Better design would be to either return some special value to indicate failure or (better) to allow the exception to propagate, or throw a different one.

Catching Exception is usually a bad idea. The problem is that you may end up catching all sorts of unexpected exceptions.

Unconditionally writing stacktraces to standard output is bad practice. Use a logging system; e.g. java.util.logging, log4j, etcetera.

Here is an alternative version that avoids these problems:

public Comics add(Comics comics) throws ... {
    EntityManager em = EMF.get().createEntityManager();
    EntityTransaction tx = null;
    try {
        tx = em.getTransaction();
        tx.begin();
        em.persist(comics);
        tx.commit();
    } finally {
        try {
            if (tx != null && tx.isActive()) {
                tx.rollback();
            }
        } finally {
            em.close();
        }
    }
    return comics; 
}

Answer 2

There is no silver bullet, no universal best practice, imho, but i would write code like that:

public Comics add(Comics comics) throws PersistenceException {
    EntityManager em = EMF.get().createEntityManager();
    // we have PersistenceContext "resource" em
    try {
        EntityTransaction tx = em.getTransaction();
        // we have "resource" tx : lifecycle have 2 final states - committed or rolled back
        try {
            tx.begin();
            em.persist(comics);
            tx.commit(); // final state 1
        } finally {
            if (tx.isActive())
                tx.rollback(); // or final state 2
        }
    // free "resource" em
    } finally {
        em.close();
    }
    return comics;
}