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.

• Thufir: Ok, I grudgingly take your point vis a vis the entity manager, but I was more asking in the general.
• palacsint: @Thufir: Sometimes you just can't answer the OP's specific question but you have some useful comment on another part of the code (like Eelke's comment here, I guess). It's fine, ontopic and encouraged on this site (according to the FAQ).

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.

• Thufir: Please expand on "Mocking non-static methods is closer to OOP and easier than static ones. " --why?
• palacsint: @Thufir: Because you don't need any trick (such as proxies, byte-code manipulation) to change the behavior of a static method for only a testcase. You can do it with pure polymorphism (which is a core OOP feature).