Why is it bad to wrap a unit of work in the Dispose () method?

Question

I wrote an implementation of UnitOfWork that does not disclose the public Commit() method. Instead, UnitOfWork implements IDisposable , and Commit is executed in the Dispose() method. I don't see any immediate problems with this, but it seems unorthodox, so I want to know if you guys can point out some main reason so as not to do this, which I miss.

Here is a sample code:

 public class DataService { public DataService() { _db = new MyDataContext(); _exceptionHandler = new SqlExceptionHandler(); } private readonly MyDataContext _db; private readonly SqlExceptionHandler _exceptionHandler; public void Add(Product product, Cart cart) { using(UnitOfWork unitOfWork = new UnitOfWork(_db, ex=>_exceptionHandler.Handle(ex))) { unitOfWork.Create<CartItem>(new CartItem{CartId = cart.Id, ProductId = product.Id}); unitOfWork.Update<Product>(x => x.Id == product.Id, product => { product.OrderCount++; }); } } } public class UnitOfWork : IDisposable { private readonly DataContext _dataContext; private readonly Func<Exception, bool> _handleException; private bool _dirty; public UnitOfWork(DataContext dataContext, Func<Exception,bool> handleException) { _dataContext = dataContext; _handleException = handleException; } private Table<T> Table<T>() where T: class { return _dataContext.GetTable<T>(); } private T[] Find<T>(Expression<Func<T,bool>> select) where T: class { return Table<T>().Where(select).ToArray(); } public void Create<T>(T persistentObject) where T: class { Table<T>().InsertOnSubmit(persistentObject); _dirty = true; } public void Update<T>(Expression<Func<T, bool>> select, Action<T> update) where T : class { var items = Find<T>(select); if (items.Length > 0) { foreach (var target in items) update(target); _dirty = true; } } public void Delete<T>(Expression<Func<T, bool>> select) where T : class { var items = Find<T>(select); switch (items.Length) { case 0: return; case 1: Table<T>().DeleteOnSubmit(items[0]); break; default: Table<T>().DeleteAllOnSubmit(items); break; } _dirty = true; } public void Dispose() { if (_dirty) { Commit(1); } } private void Commit(int attempt) { try { _dataContext.SubmitChanges(); } catch (Exception exception) { if (attempt == 1 && _handleException != null && _handleException(exception)) { Commit(2); } else { throw; } } } } 

Answer 1

Because the unhandled exception will commit the transaction. And the exception implies that something did not go as planned = transaction should not be executed.

This is much better than Rollback in Dispose if Commit not called before uninstall.

Answer 2

What if one of your functions called in the using block throws an exception? This may cause your unit of work to be in an inconsistent / incomplete state, which you then complete.