Problem
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.
Solution
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;
}
}