3
\$\begingroup\$

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?

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Ok, I grudgingly take your point vis a vis the entity manager, but I was more asking in the general. \$\endgroup\$ – Thufir Aug 5 '12 at 23:41
  • \$\begingroup\$ @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). \$\endgroup\$ – palacsint Aug 6 '12 at 10:18
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Please expand on "Mocking non-static methods is closer to OOP and easier than static ones. " --why? \$\endgroup\$ – Thufir Aug 5 '12 at 23:34
  • \$\begingroup\$ @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). \$\endgroup\$ – palacsint Aug 6 '12 at 10:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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