current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite

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.

share|improve this question
    
see meta.codereview.stackexchange.com/questions/598/… for tag synonym request. – antony.trupe Sep 18 '12 at 23:47
up vote 5 down vote accepted
  • 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.

share|improve this answer
    
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. – Eugen Martynov Sep 18 '12 at 16:40
    
But looks like it is :) so just ignore my comments :) – Eugen Martynov Sep 18 '12 at 16:41
up vote 5 down vote

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();
    }
}
share|improve this answer
    
if return immediately, em.close will not be executed? – JR Galia Sep 18 '12 at 13:22
3  
@JRGalia the finally block is always executed. – codesparkle Sep 18 '12 at 13:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.