8
\$\begingroup\$

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?

\$\endgroup\$
0

2 Answers 2

5
\$\begingroup\$

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; 
}
\$\endgroup\$
3
  • \$\begingroup\$ should it throws Exception? \$\endgroup\$
    – JR Galia
    Sep 10, 2012 at 5:14
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Stephen C
    Sep 12, 2012 at 8:07
  • \$\begingroup\$ why can't we just removed the nested try and finally in finally block? \$\endgroup\$
    – JR Galia
    Sep 18, 2012 at 10:32
3
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ is the nested try and finally block necessary? \$\endgroup\$
    – JR Galia
    Sep 19, 2012 at 8:58
  • \$\begingroup\$ Welcome to Code Review! Please try to explain the choices behind the code: the code itself is not enough here IMHO. \$\endgroup\$ Sep 21, 2012 at 14:08

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.