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; 
}

• JR Galia: should it throws Exception?
• Stephen C: No ... it should list the checked exceptions that are not being caught. Declaring a method as throwing Exception means that the caller has no clue what to expect.
• JR Galia: why can't we just removed the nested try and finally in finally block?

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;
}

• JR Galia: is the nested try and finally block necessary?
• Quentin Pradet: Welcome to Code Review! Please try to explain the choices behind the code: the code itself is not enough here IMHO.