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

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 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);
}
Arti

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.

StackExchange Code Review Q#14347, answer score: 5