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\$
1

2 Answers 2

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\$
2
  • \$\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\$ Commented Sep 18, 2012 at 16:40
  • \$\begingroup\$ But looks like it is :) so just ignore my comments :) \$\endgroup\$ Commented Sep 18, 2012 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\$
2
  • \$\begingroup\$ if return immediately, em.close will not be executed? \$\endgroup\$ Commented Sep 18, 2012 at 13:22
  • 3
    \$\begingroup\$ @JRGalia the finally block is always executed. \$\endgroup\$ Commented Sep 18, 2012 at 13:24

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.