HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaMinor

Coding style and best practice regarding exception-handling

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handlingexceptionpracticeregardingstyleandcodingbest

Problem

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?

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#15467, answer score: 5

Revisions (0)

No revisions yet.