5
\$\begingroup\$

Below is my method in checking if entity already exist.

public boolean isExist(String genreName) throws PersistenceException{
    EntityManager em = EMF.get().createEntityManager();
    boolean flag = false;

    try{
        Genre genre = em.find(Genre.class, genreName);
        if (genre != null)
            flag = true;

    }finally{
        em.close();
    }

    return flag;
}

Is the code above ok? Please suggest.

\$\endgroup\$
5
\$\begingroup\$
  • isExist is not English. Call it something like genreExists.
  • In addition to omitting the flag, as palacsint suggested, you can get rid of the if branch:

public boolean genreExists(String genreName) throws PersistenceException {
    EntityManager em = EMF.get().createEntityManager();
    try {
        return em.find(Genre.class, genreName) != null;
    } finally {
        em.close();
    }
}

Probably irrelevant side note:

If you were using Java 7, and EntityManager implemented AutoCloseable or one of its various subinterfaces (such as Closeable), your code could be even cleaner using the new try-with-resources statement:

try (EntityManager em = EMF.get().createEntityManager()) {
    return em.find(Genre.class, genreName) != null;
}

But it doesn't, so you can't.

\$\endgroup\$
  • \$\begingroup\$ I'm not familiar with GAE, don't know if EMF.get().createEntityManager() is mockable. If not there could be difficulties if we want to cover this function with unit tests. \$\endgroup\$ – Eugen Martynov Sep 18 '12 at 16:40
  • \$\begingroup\$ But looks like it is :) so just ignore my comments :) \$\endgroup\$ – Eugen Martynov Sep 18 '12 at 16:41
5
\$\begingroup\$

It looks fine. You could omit the boolean flag if you return immediately when you know the answer:

public boolean isExist(final String genreName) throws PersistenceException {
    final EntityManager em = EMF.get().createEntityManager();
    try {
        final Genre genre = em.find(Genre.class, genreName);
        if (genre != null) {
            return true;
        }
        return false;
    } finally {
        em.close();
    }
}
\$\endgroup\$
  • \$\begingroup\$ if return immediately, em.close will not be executed? \$\endgroup\$ – JR Galia Sep 18 '12 at 13:22
  • 3
    \$\begingroup\$ @JRGalia the finally block is always executed. \$\endgroup\$ – Adam Sep 18 '12 at 13:24

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.