5
\$\begingroup\$

I have written two methods that executes any type of HQL and returns data. Methods are like

public static Query getHqlQueryObject(String hql) throws Exception{

        Query query = null;
        Session session = null;
        try {
            session = HibernateUtil.getSessionFactory().getCurrentSession();
            Transaction transaction = session.beginTransaction();
            query = session.createQuery(hql);
        } catch (Exception e) {
            System.out.println("Exception in Helper::getHqlQueryObject");
            e.printStackTrace();
        }
        return query;
    }

    public static List<Object> getHqlQueryData(Query query) throws Exception{
        List dataList = new ArrayList();
        Session session = null;
        try {
            session = HibernateUtil.getSessionFactory().getCurrentSession();
            Transaction transaction = session.getTransaction();
            dataList = query.list();
            transaction.commit();
        } catch (Exception e) {
            System.out.println("Exception in Helper::getHqlQueryData");
            e.printStackTrace();
        }
        return dataList;
    }

I am calling this methods like this

Query query = Helper.getHqlQueryObject("from BookDetails where pkBookId = :pkBookId");
query.setParameter("pkBookId", new Long(1));
List dataList = Helper.getHqlQueryData(query);

My question is, is it right approach? or should I write session initialization code every time when I want to execute a query?

\$\endgroup\$
  • \$\begingroup\$ I don't know much about Hibernate, but what happens to the transaction? \$\endgroup\$ – Tassos Bassoukos Aug 27 '14 at 9:37
  • \$\begingroup\$ Just catching and printing the Exception is not a good idea. You should at least re-throw it. \$\endgroup\$ – Florent Bayle Aug 27 '14 at 9:38
  • \$\begingroup\$ @Tassos Bassoukos i did not get your question. Code is working fine. I just want to be sure this approach is good or not. \$\endgroup\$ – Lalit Chattar Aug 27 '14 at 9:38
  • \$\begingroup\$ what tassos means is, that you open up a transaction in one method and close it in another, but noone assures that both methods are called in union. However, the Session should be intialized once (notice that you just acquire current session, you dont initialize it in your code, so thats fine) and transaction might be joined sometimes. However: Helper.getHqlQueryObject("from BookDetails where pkBookId = :pkBookId").setParameter("pkBookId", new Long(1)).list(); should work as well, so i dont get the sense of your second method. \$\endgroup\$ – user2504380 Aug 27 '14 at 12:54
  • \$\begingroup\$ @user2504380 I am calling getHqlQueryData in different class and i will be in need of transaction object to commit transaction. \$\endgroup\$ – Lalit Chattar Aug 27 '14 at 16:19
6
\$\begingroup\$

or should i write session initialization code every time when i want to execute query?

Sessions are meant to be re-used. You should not initialize a new method every time. I believe HibernateUtil.getSessionFactory().getCurrentSession(); is OK to use as that re-uses an existing session.

As for the rest of your code, I have a couple of things to say.

public static Query getHqlQueryObject(String hql) throws Exception
public static List<Object> getHqlQueryData(Query query) throws Exception

Neither of these methods actually throw an Exception. Because you have this code:

} catch (Exception e) {
    System.out.println("Exception in Helper::...");
    e.printStackTrace();
}

THIS IS BAD! A caller of your method has absolutely no guarantee that nothing went wrong. And is therefore unable to tell any potential user of your application that something did went wrong.

Simply printing the stacktrace accomplishes nothing for robustness!

Additionally, the only exception that should be able to occur is HibernateException, there is no need to catch all the other ones.


Additionally, there is no need for Transaction transaction = session.beginTransaction(); in your getHqlQueryObject. The transaction is only needed when the Query is performed, not when it is created.

Likewise, your getHqlQueryData are not guaranteed that a transaction is active.


I also have to question the usefulness of your code. Hibernate is already an abstraction and does a great job in simplifying things. What is it exactly you want to make easier with these methods?

Your current code:

Query query = Helper.getHqlQueryObject("from BookDetails where pkBookId = :pkBookId");
query.setParameter("pkBookId", new Long(1));
List dataList = Helper.getHqlQueryData(query);

How I would do it:

try {
    Query query = session.createQuery("from BookDetails where pkBookId = :pkBookId");
    query.setParameter("pkBookId", 1L);
    Transaction transaction = session.getTransaction();
    List<?> dataList = query.list();
    transaction.commit();
}
catch (HibernateException ex) {
    if (transaction.isActive()) {
         transaction.rollback();
    }
    // either re-throw the exception, wrap in another exception, or return a value indicating error.
}

You do have a session variable available here already, I hope? If not, you should remember the principle Tell, don't ask. That is, if your code needs some information, you should pass it there. I generally don't recommend calling a public static method or similar to get access to it.

By the way, I am not so sure that query.list(); needs an active transaction.

Also, when a transaction fails (an exception is thrown), you should call transaction.rollback();

And the last thing I'd like to say, with your HQL being "from BookDetails where pkBookId = :pkBookId" there is no need to return a list, you can instead call the method uniqueResult

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. I was trying to write a method where i can pass the query and it returns data. I do not want to write same code again and again in all controller class because it might be possible i execute multiple query in same class. \$\endgroup\$ – Lalit Chattar Aug 28 '14 at 14:47
  • \$\begingroup\$ @LalitChattar Sounds like you should try to look at what things are really the common things. If you need help, create a new question with your real code that you are writing over and over again and we might be able to help you. There are some things that are OK to write over and over again though. \$\endgroup\$ – Simon Forsberg Aug 28 '14 at 16:55
1
\$\begingroup\$

Few points to mention here

  1. You should not be catching exceptions unless you are doing some business logic in catch block.

  2. Getting session instance in getHqlQueryObject() and getHqlQueryData() is redundant, should avoid this. With a connection pool, the overhead to open and close session isn't big. What takes time is to construct the session factory. Anyways, try to merge the above two methods into single.

  3. Anyways you need to open a session, while setting query parameters, as some might be lazy and need to initialize it. This will be at your implementation level, whoever is calling your service.

| improve this answer | |
\$\endgroup\$

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.