What's wrong with static utility classes, versus beans?

Question

My inclination is to make these methods static:

package net.bounceme.dur.usenet.driver;

import java.util.List;
import java.util.logging.Logger;
import javax.mail.Folder;
import javax.mail.Message;
import javax.persistence.*;
import net.bounceme.dur.usenet.model.Article;
import net.bounceme.dur.usenet.model.Newsgroup;

class DatabaseUtils {

    private static final Logger LOG = Logger.getLogger(DatabaseUtils.class.getName());
    private EntityManagerFactory emf = Persistence.createEntityManagerFactory("USENETPU");
    private EntityManager em = emf.createEntityManager();

    public int getMax(Folder folder) {
        int max = 0;
        String ng = folder.getFullName();
        String queryString = "select max(article.messageNumber) from Article article left join article.newsgroup newsgroup where newsgroup.newsgroup = '" + ng + "'";
        try {
            max = (Integer) em.createQuery(queryString).getSingleResult();
        } catch (Exception e) {
            LOG.info("setting max to zero");
        }
        LOG.severe(folder.getFullName() + "\t" + max);
        return max;
    }

    public void persistArticle(Message message, Folder folder) {
        em.getTransaction().begin();
        String fullNewsgroupName = folder.getFullName();
        Newsgroup newsgroup = null;
        int max = getMax(folder);
        TypedQuery<Newsgroup> query = em.createQuery("SELECT n FROM Newsgroup n WHERE n.newsgroup = :newsGroupParam", Newsgroup.class);
        query.setParameter("newsGroupParam", fullNewsgroupName);
        try {
            newsgroup = query.getSingleResult();
            LOG.fine("found " + query.getSingleResult());
        } catch (javax.persistence.NoResultException e) {
            LOG.fine(e + "\ncould not find " + fullNewsgroupName);
            newsgroup = new Newsgroup(folder);
            em.persist(newsgroup);
        } catch (NonUniqueResultException e) {
            LOG.warning("\nshould never happen\t" + fullNewsgroupName);
        }
        Article article = new Article(message, newsgroup);
        em.persist(article);
        em.getTransaction().commit();
    }

    public void close() {
        em.close();
        emf.close();//necessary?
    }
}

However, I'm quite sure that I'm in the minority! Why?

A quick look at Math shows that this isn't a strange or odd approach. The object itself keeps no state, really, so why would a bean or POJO be preferred to static methods?

Answer 1

Actually the object contains an entity manager which has state. In a simple application having only one shared entity manager might work but if you made this into an EJB in a Java-EE application you would definitely want each request to use it's own entity manager.

Answer 2

Static helper classes makes testing harder. Nick Malik has a good article on this topic: Killing the Helper class, part two.

Mocking non-static methods is closer to OOP and easier than static ones. Another gain is that you'll have simple tests: simple tests for the utility/helper class and simple tests for the clients of the utility class; the tests of the client classes don't have to know the internals of the utility class, and you don't need a DatabaseUtilsTestHelper class to keep in one place the mocking logic for the helper class. The linked article is worth the reading.

Some other notes:

• Try not using short variable names like ng. They are hard to read.

• persistArticle does not close the transaction in every path. You should rollback it in the finally block if it's still active:

  final EntityTransaction transaction = em.getTransaction();
  transaction.begin();
  try {
      ...    
      transaction.commit();
  } finally {
      if (transaction.isActive()) {
          transaction.rollback();
      }
  }
  

• persistArticle does not use the value of max, therefore the getMax call seems unnecessary.