0
\$\begingroup\$

I have created a dataAccessLayer for nHibernate. I suspect there are some bugs in the code. I am really new to nHibernate. Can anyone suggest me the best practice for creating dataAccessLayer. I implemented counter in beginTransaction and commitTransaction to apply nested transactions.

My dataAccessLayer :

using NHibernate;
using NHibernate.Cfg;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;

namespace CafePOS.Library
{
    public class HibernateUtils
    {
        private static int counter = 0;
        private static volatile ISessionFactory iSessionFactory;
        private static volatile ITransaction iTransaction;
        private static HibernateUtils instance = null;
        public static ISession session = null;
        private static object syncRoot = new Object();

        private HibernateUtils()
        {
            //configure
            Configuration configuration = new Configuration().Configure("hibernate.cfg.xml");
            Assembly assembly = Assembly.GetCallingAssembly();
            configuration.AddAssembly(assembly);
            iSessionFactory = configuration.BuildSessionFactory();
            session = iSessionFactory.OpenSession();
        }

        public static HibernateUtils Instance
        {
            get
            {
                if (instance == null)
                {
                    lock (syncRoot)
                    {
                        if (iSessionFactory == null)
                        {
                            instance = new HibernateUtils();
                        }
                    }

                }
                return instance;
            }
        }
        public  void closeSessionFactory()
        {
            if (iSessionFactory != null)
                iSessionFactory.Close();
        }

        public  ISession GetCurrentSession()
        {
            if (session != null)
                return session;
            HibernateUtils.instance.GetCurrentSession();
            return session;
        }

        public void beginTransaction()
        {
            try
            {
                counter++;
                if (iSessionFactory != null)
                {
                    iTransaction = session.BeginTransaction();
                }

            }
            catch (Exception ex)
            {

                throw ex;
            }
        }

        public void commitTransaction()
        {
            try
            {
                if (counter == 1 && iTransaction != null)
                {
                    iTransaction.Commit();
                    iTransaction = null;
                    session.Dispose();
                }

                counter--;
            }
            catch (Exception ex)
            {

                throw ex;
            }
        }

        public void rollbackTransaction()
        {
            try
            {
                if (iTransaction != null)
                {
                    iTransaction.Rollback();
                }
            }
            catch (Exception ex)
            {
                throw ex;
            }
        }
    }
}

An example using this dataAccessLayer is:

public static string GetTablePosition(string table_id)
{
    try
    {
        ISession session = Library.HibernateUtils.Instance.GetCurrentSession();
        Library.HibernateUtils.Instance.beginTransaction();
        CafeTable _table = new CafeTable();
        _table = session.Get<CafeTable>(Convert.ToDecimal(table_id));
        string table_position = _table.table_position.ToString();
        Library.HibernateUtils.Instance.commitTransaction();
        return table_position;
    }
    catch (Exception ex)
    {
        Library.HibernateUtils.Instance.beginTransaction();
        throw ex;
    }
}

The class GetTablePosition() is consumed here:

public static Boolean DeleteTable(string table_id)
{
    try
    {
        ISession session = Library.HibernateUtils.Instance.GetCurrentSession();
        Library.HibernateUtils.Instance.beginTransaction();
        CafeTable _group = session.Get<CafeTable>(Convert.ToDecimal(table_id));
        decimal table_pos = Convert.ToDecimal(GetTablePosition(table_id));
        //changing all table position below 
        session.CreateQuery("UPDATE  CafeTable set table_position=(table_position-1) WHERE table_position>:table_position").SetParameter("table_position", table_pos).ExecuteUpdate();

        session.Delete(_group);
        Library.HibernateUtils.Instance.commitTransaction();
        return true;
    }
    catch (Exception ex)
    {
        Library.HibernateUtils.Instance.rollbackTransaction();
        throw ex;
    }
}

I would be thankful if anyone can help me find out flaw in this class.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you fix the code and either remove or comment back in the parts that are currently commented out? It's not clear what purpose they have and it's confusing. I have the impression that this is not your real code but edited for the question. \$\endgroup\$ – t3chb0t Nov 1 '17 at 10:21
  • \$\begingroup\$ @t3chb0t sir that was my real code and i didn't think of removing out the commented lines. Now I have removed those. Can you please check it out once? \$\endgroup\$ – Hacked user Nov 1 '17 at 10:27
  • \$\begingroup\$ Ok, looks fine to me now. Do you have a usage example and could post one too? I guess you are using it somewhere. I'm curious how. \$\endgroup\$ – t3chb0t Nov 1 '17 at 10:38
  • \$\begingroup\$ @t3chb0t sir I have added an example too for deleting a cafe_table. Can you please look at it? Is there any flaw in this class? Do using this class affect any things in nHibernate built-in dataAccessLayer? \$\endgroup\$ – Hacked user Nov 1 '17 at 10:55
1
\$\begingroup\$

Excepiton handling

Almost all of your try/catch blocks are meaningless (at least those with only throw ex;). If all you do is to rethrow the exeption then it's better to do without them. But in your case however they are even harmful. Instead of just doing throw; you do throw ex; which distroys the original stack-trace and creates a new one so debugging your code wouldn't reveal the real cause of the error.


Inconsistent code

The naming is very incosistent and does not help understand the code, actually it raises even more questions. It does not follow any known naming convention but mixes several of them. It uses prefixes for some variables but not for others. It mixes PascalCase and camelCase and it looks like it was written by three different developers each using a different naming style.

There is nowhere a factory (as a public API) but you call a method closeSessionFactory so the obvious question when I see this method is: What session factory? Did I create one? Does it really need to be public?


catch (Exception ex)
{
    Library.HibernateUtils.Instance.beginTransaction();
    throw ex;
}

The most weird part of it is this catch where you beginTransaction. This is so unconventional that I cannot think of any resonable explanation for it. Usually you roll everything back there and not begin a new transaction.

Overall I think this implementation is very confusing and I'm not sure what is the actual benefit of it.


Bug?

I think there is a bug in your code.

public void commitTransaction()
{
    try
    {
        if (counter == 1 && iTransaction != null)
        {
            iTransaction.Commit();
            iTransaction = null;
            session.Dispose();
        }

        counter--;
    }
    catch (Exception ex)
    {

        throw ex;
    }
}

commitTransaction disposes the session but the only place where it is assigned is the constructor. Later you just check if it's null in

public  ISession GetCurrentSession()
{
    if (session != null)
        return session;
    HibernateUtils.instance.GetCurrentSession();
    return session;
}

But after being disposed it is not set to null so using it agian after that would raise an object disposed exception. Or at least it should unless the session's Dispose doesn't really do anything useful so your code continues to work despite session having been disposed.


How would a better API look like? If I was to use your helper I would expect it to work like this:

CurrentSession is the instance of the singleton and it provides all APIs that I need to peform a database operation. Begin/Comming/RollbackTransaction and Get<T>. I find there is no need for the intermediate Instance property and then again call GetCurrentSession. The helper should handle all the additional calls already or otherwise it's not much of a helper.

public static string GetTablePosition(string tableId)
{
    try
    {        
        HibernateUtils.CurrentSession.BeginTransaction();
        CafeTable _table = HibernateUtils.CurrentSession.Get<CafeTable>(Convert.ToDecimal(tableId));
        string tablePosition = _table.tablePosition.ToString();
        HibernateUtils.CurrentSession.CommitTransaction();
        return tablePosition;
    }
    catch (Exception)
    {
        HibernateUtils.CurrentSession.RollbackTransaction();
        throw;
    }
}
\$\endgroup\$
  • \$\begingroup\$ sorry sir that should have been rollbackTransaction() instead of beginTransaction instead of beginTransaction in catch block \$\endgroup\$ – Hacked user Nov 2 '17 at 4:00
  • \$\begingroup\$ sir will that help me in nested transactions if I use code as you suggested in GetTablePosition() method? \$\endgroup\$ – Hacked user Nov 2 '17 at 5:06
  • \$\begingroup\$ will it be fine if I used session=null; in commitTransaction() instead of session.dispose()? \$\endgroup\$ – Hacked user Nov 2 '17 at 5:59
  • \$\begingroup\$ @Hackeduser instead? I've got the impression that you don't really know what you are doing ;-) perhaps it'd be best if you first read a little about what is disposing any why we are doing this etc. \$\endgroup\$ – t3chb0t Nov 2 '17 at 6:59

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.