4
\$\begingroup\$

I am trying to use Entity framework along with UOW in business class where I want to use this layer across multiple types of applications like web, webapi and Windows Services. So I cooked something from the online resources. Can some one figure out any potential performance or fallback?

Main reason I am trying to write this way is to use this across multiple application layers with out dealing Entities across them. Now all I have to do is add reference to the application and use them as written at the bottom.

 public interface IUnitOfWork : IDisposable
    {

        /// <summary>
        /// Call this to commit the unit of work
        /// </summary>
        void Commit();

        /// <summary>
        /// Return the database reference for this UOW
        /// </summary>
        DbContext Db { get; }

        /// <summary>
        /// Starts a transaction on this unit of work
        /// </summary>
        void StartTransaction();


    }


public interface IBaseRepository<T>
    {
        /// <summary>
        /// Retrieve a single item using it's primary key, exception if not found
        /// </summary>
        /// <param name="primaryKey">The primary key of the record</param>
        /// <returns>T</returns>
        T Single(object primaryKey);
        /// <summary>
        /// Retrieve created by and modified by id's FullName
        /// </summary>
        /// <param name="dynamicObject">The primary key of the record</param>
        /// <returns>T</returns>
        Dictionary<string, string> GetAuditNames(dynamic dynamicObject);
        /// <summary>
        /// Retrieve a single item by it's primary key or return null if not found
        /// </summary>
        /// <param name="primaryKey">Prmary key to find</param>
        /// <returns>T</returns>
        T SingleOrDefault(object primaryKey);

        /// <summary>
        /// Returns all the rows for type T
        /// </summary>
        /// <returns></returns>
        IEnumerable<T> GetAll();

        /// <summary>
        /// Does this item exist by it's primary key
        /// </summary>
        /// <param name="primaryKey"></param>
        /// <returns></returns>
        bool Exists(object primaryKey);

        /// <summary>
        /// Inserts the data into the table
        /// </summary>
        /// <param name="entity">The entity to insert</param>
        /// <param name="userId">The user performing the insert</param>
        /// <returns></returns>
        void Insert(T entity);

        /// <summary>
        /// Updates this entity in the database using it's primary key
        /// </summary>
        /// <param name="entity">The entity to update</param>
        /// <param name="userId">The user performing the update</param>
        void Update(T entity);



        /// <summary>
        /// Deletes this entry fro the database
        /// ** WARNING - Most items should be marked inactive and Updated, not deleted
        /// </summary>
        /// <param name="entity">The entity to delete</param>
        /// <param name="userId">The user Id who deleted the entity</param>
        /// <returns></returns>
        int Delete(T entity);

        IUnitOfWork UnitOfWork { get; }



    }

Abstract Classes

public class AbstractUnitOfWork : IUnitOfWork
    {
         private TransactionScope _transaction;
        private DbContext _db;


        public AbstractUnitOfWork(DbContext targetContext)
        {
            _db = targetContext;

        }

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (disposing)
            {
                if (_db != null)
                {
                    _db.Dispose();
                }
            }
        }

        public void StartTransaction()
        {
            _transaction = new TransactionScope();
        }

        public void Commit()
        {
            _db.SaveChanges();
            _transaction.Complete();
        }

        public DbContext Db
        {
            get { return _db; }
        }
    }




 public abstract class BaseRepository<T> : IBaseRepository<T>
        where T : class
    {
        private  IUnitOfWork _unitOfWork;
        internal DbSet<T> dbSet;

        public BaseRepository()
        {
        }

        public BaseRepository(IUnitOfWork unitOfWork)
        {
            if (unitOfWork == null) throw new ArgumentNullException("unitOfWork");
            _unitOfWork = unitOfWork;
            this.dbSet = _unitOfWork.Db.Set<T>();
        }


        public BaseRepository(DbContext targetContext)
        {
            if (targetContext == null) throw new ArgumentNullException("TargetContext");
            _unitOfWork = new AbstractUnitOfWork(targetContext);
            this.dbSet = _unitOfWork.Db.Set<T>();            
        }

        /// <summary>
        /// Returns the object with the primary key specifies or throws
        /// </summary>
        /// <typeparam name="TU">The type to map the result to</typeparam>
        /// <param name="primaryKey">The primary key</param>
        /// <returns>The result mapped to the specified type</returns>
        public T Single(object primaryKey)
        {

            var dbResult = dbSet.Find(primaryKey);
            return dbResult;

        }


        public IEnumerable<T> Find(Expression<Func<T, bool>> where)
        {
            return _unitOfWork.Db.Set<T>().AsNoTracking<T>().Where(where);
        }



        /// <summary>
        /// Returns the object with the primary key specifies or the default for the type
        /// </summary>
        /// <typeparam name="TU">The type to map the result to</typeparam>
        /// <param name="primaryKey">The primary key</param>
        /// <returns>The result mapped to the specified type</returns>
        public T SingleOrDefault(object primaryKey)
        {
            var dbResult = dbSet.Find(primaryKey);
            return dbResult;
        }





        public bool Exists(object primaryKey)
        {
            return dbSet.Find(primaryKey) == null ? false : true;
        }

        public virtual void Insert(T entity)
        {
            dynamic obj = dbSet.Add(entity);
            this._unitOfWork.Db.SaveChanges();
            //return obj.ID;
        }

        public virtual void Update(T entity)
        {
            dbSet.Attach(entity);
            _unitOfWork.Db.Entry(entity).State = EntityState.Modified;
            this._unitOfWork.Db.SaveChanges();
        }


        public virtual void Insert(DbContext targetContext)
        {
            if (targetContext != null)
            {               
                this.dbSet = targetContext.Set<T>();
            }
            if (dbSet == null)
            {
                throw new ArgumentNullException("No Unit of Work Context available for processing the CRUD Operation");
            }
            dynamic obj = dbSet.Add((T)(object)this);           
            targetContext.SaveChanges();
        }


        public virtual void Update(DbContext targetContext)
        {
            if (targetContext != null)
            {               
                this.dbSet = targetContext.Set<T>();
            }
            if (dbSet ==null)
            {
                throw new ArgumentNullException("No Unit of Work Context available for processing the CRUD Operation");
            }
            dbSet.Attach((T)(object)this);            
            targetContext.Entry((T)(object)this).State = EntityState.Modified;
            targetContext.SaveChanges();

        }


        public virtual int Delete(DbContext targetContext)
        {
            if (targetContext != null)
            {
                _unitOfWork = new AbstractUnitOfWork(targetContext);
                this.dbSet = _unitOfWork.Db.Set<T>();
            }
            if (dbSet == null)
            {
                throw new ArgumentNullException("No Unit of Work Context available for processing the CRUD Operation");
            }
            if (_unitOfWork.Db.Entry((T)(object)this).State == EntityState.Detached)
            {
                dbSet.Attach((T)(object)this);
            }
            dynamic obj = dbSet.Remove((T)(object)this);
            this._unitOfWork.Db.SaveChanges();
            return obj.Id;
        }

        public int Delete(T entity)
        {
            if (_unitOfWork.Db.Entry(entity).State == EntityState.Detached)
            {
                dbSet.Attach(entity);
            }         
            dynamic obj = dbSet.Remove(entity);
            this._unitOfWork.Db.SaveChanges();
            return obj.Id;
        }
        public IUnitOfWork UnitOfWork { get { return _unitOfWork; } }
        internal DbContext Database { get { return _unitOfWork.Db; } }
        public Dictionary<string, string> GetAuditNames(dynamic dynamicObject)
        {
            throw new NotImplementedException();
        }

        public IEnumerable<T> GetAll()
        {
            return dbSet.AsEnumerable().ToList();
        }

    }

Entity Classes (Partial classes of the edmx generated)

 partial class MyClass : BaseRepository<MyClass>
{
    public MyClass(DbContext context):base(context)
    {


    }
    public static MyClass GetByOrgID(long id)
    {
        return MyClass.Instance.Find(org => org.OrganizationID == id).FirstOrDefault();
    }

    public static MyClass GetByOrgGuid(Guid orgGuid)
    {
        return MyClass.Instance.Find(org => org.OrgGuid == orgGuid).FirstOrDefault();
    }

    public static MyClass GetOrgDetailsbyEmail(string email)
    {
        return MyClass.Instance.Find(o => o.OrgEmail == email).FirstOrDefault();
    }

    public static MyClass[] GetEverythingAll()
    {
        return MyClass.Instance.GetAll().ToArray();
    }

    public static bool IsEmailAlreadyExists(string email)
    {
        return (MyClass.Instance.Find(org => org.OrgEmail == email).FirstOrDefault() != null ? true : false);
    }

    public static long GetOrgIdbyEmail(string email)
    {
        long orgId = 0;
        MyClass org = MyClass.Instance.Find(o => o.OrgEmail == email).FirstOrDefault();
        if(org != null)
            orgId = org.OrganizationID;

        return orgId;
    }

    public static long GetOrgIdbyGuid(Guid orgGuid)
    {
        long orgId = 0;
        MyClass org = MyClass.Instance.Find(o => o.OrgGuid == orgGuid).FirstOrDefault();
        if (org != null)
            orgId = org.OrganizationID;

        return orgId;
    }    

    #region Singleton

    private static MyClass _instance = null;
    private static readonly object _padlock = new object();
    public static MyClass Instance
    {
        get
        {
            if (_instance == null)
            {
                lock (_padlock)
                {
                    if (_instance == null)
                    {
                        _instance = new MyClass(new OrganizationEntities());
                    }
                }
            }
            return _instance;
        }
    }


    #endregion Singleton

}

Retrieving and updating records from any application layer looks like

/Getting database record

        MyClass.Instance.GetByOrgID(20);

        // Save record code looks like
        private readonly DbContext _uowContext = new OrganizationEntities();
        MyClass newInstance = new MyClass();
        newInstance.OrgGuid = Guid.NewGuid();
        newInstance.OrgName = reqOrg.OrgName;                
        newInstance.OrgEmail = reqOrg.OrgEmail;
        newInstance.OrgPhoneNumber1 = reqOrg.OrgPhoneNumber1;
        newInstance.OrgPhoneNumber2 = reqOrg.OrgPhoneNumber2;
        newInstance.OrgFaxNumber = reqOrg.OrgFaxNumber;                
        newInstance.IsActive = true;                
        newInstance.Insert(_uowContext);
\$\endgroup\$
3
\$\begingroup\$

I think it's wrong. Your entity types are mixing concerns and are no longer simple POCO types, they now have incestuous knowledge of a DbContext (which can access every single entity type and the database itself, run stored procedures, ...everything).

Not only that, but you're working against Entity Framework, rather than with it: a DbContext is a unit-of-work, as it already encapsulates a transaction for you - and regardless of what people say, IDbSet<T> is a repository, at least as far as EF is concerned.

Consider:

public interface IUnitOfWork : IDisposable
{
    void Commit();
    IDbSet<T> Repository<T>();
}

A DbContext can very easily implement that interface:

public class MyDbContext : DbContext, IUnitOfWork
{
    /* ...implementation details... */

    public IDbSet<T> Repository<T>()
    {
        return Set<T>();
    }

    public void Commit()
    {
        SaveChanges();
    }
}

Needing a StartTransaction method on a UoW feels weird at best, because the sole purpose of a UoW is to encapsulate a transaction - in an ideal world, you would only call SaveChanges once in its entire lifetime.


Your "abstract unit of work" isn't an abstract class, which makes things confusing.

Entity types should be Plain Old CLR Objects - simple classes with getters+setters; yours are full-fledged data access services, and to be honest I'm surprised Entity Framework can even work with those, given the absence of a default constructor and... the singleton.

How are you supposed to have an IEnumerable<TEntity> if TEntity is a singleton?

How does this even compiles without a default constructor?

MyClass newInstance = new MyClass();

I'm guessing the default constructor is defined by the EDMX generated code, otherwise your code would never work. Point is, if a type has to have a default constructor, even considering making a singleton out of it is pointless.

Split responsibilities, let entity types be entity types, and write your GetFooByBar methods in some FooService class that receives an IUnitOfWork dependency in its constructor:

public class SomeService
{
    private readonly IUnitOfWork _uow;

    public SomeService(IUnitOfWork uow)
    {
        _uow = uow;
    }

    public SomeEntity GetById(int id)
    {
        return _uow.Repository<SomeEntity>().SingleOrDefault(e => e.Id == id);
    }

    // ...
}

That way whoever is controlling the instantiation of SomeService gets to inject the IUnitOfWork, and if there are 20 such services, they can all get the same instance, and thus contribute to the same transaction.

\$\endgroup\$
  • \$\begingroup\$ How does this even compiles without a default constructor? MyClass newInstance = new MyClass(); I am using it as partial class which is already an edmx entity. \$\endgroup\$ – Krishna Sep 8 '16 at 18:39
  • \$\begingroup\$ @Krishna public MyClass(DbContext context):base(context) is the only constructor listed in MyClass; if the other half of the partial class didn't define a public parameterless constructor, var foo = new MyClass(); could not compile. \$\endgroup\$ – Mathieu Guindon Sep 8 '16 at 18:41
  • \$\begingroup\$ That constructor is auto generated by EDMX in partial class. Infact it is working. But I see what you are coming from. Very good points. Anyway, Can you suggest if there is a best way to encapsulate all these stuff into business library? I want to reuse it. I dont want to tag to a service. \$\endgroup\$ – Krishna Sep 8 '16 at 18:49
  • \$\begingroup\$ That constructor entirely defeats the purpose of a singleton. As for exposing the functionality, it entirely depends on your architecture. I'm not sure what you mean with "I don't want to tag to a service"... \$\endgroup\$ – Mathieu Guindon Sep 8 '16 at 18:56
  • 1
    \$\begingroup\$ You can do that if you need to - but not on an entity type. An entity isn't responsible to query, it's responsible for holding an entity's property values, period. And there needs to be more than one single instance. Hence, you need to remove the "repository" functionality from the entity types. \$\endgroup\$ – Mathieu Guindon Sep 8 '16 at 19:44
1
\$\begingroup\$

Whether or not you should set up a UoW/Repository layer on top of the UoW/Repository implementations that EF offers out of the box (DbContext/DbSet) is a question I consider beyond the scope of this review. You'll find many many opinions about this on the internet.

But a couple of things are definitely wrong.

Repository's internals

  1. I don't understand why repositories insert, update and delete themselves. For example in the Insert method, you have:

    dynamic obj = this.dbSet.Add((T)(object)this);
    

    What is this? This is either a copy/paste error or a major design flaw. How can a repository add itself to the DbSet it contains? And why the double(!) cast, why the dynamic? "Smelly" is an understatement.

  2. Why is BaseRepository's member dbSet internal? The repo is supposed to encapsulate a DbSet. What should other classes in the assembly do with it? If you need it to be internal, it's because of something you don't show, and which, again, would be a major design flaw.

  3. One Find method returns untracked entities? (AsNoTracking), while Single and GetAll methods don't. That's confusing. It may be useful to have options for applying AsNoTracking or not, but a consumer of a repository should not have to know which methods do and which don't. Either the method name should reveal it, or it should be parametrized.

  4. GetAll in its present form is a useless method. It returns ...

    this.dbSet.AsEnumerable().ToList()
    

    ... that is: the entire database table. Other than for small tables, who's going to want that?

  5. Don't save changes. Repositories shouldn't call SaveChanges. The Unit of Work is responsible for that. I know that you intend to wrap each repository operation into a TransactionScope, managed by the UoW, but if you remove these SaveChanges, the UoW doesn't need this TransactionScope: the single SaveChanges call in its Commit method will be enough to save all changes in one transaction.

Repository's public methods

  1. As a loose definition, repositories are supposed to expose the database tables as collection-like interfaces to the application code. So it's expected for them to have Insert, Update, and Delete methods. But what's the use of a method like Insert(DbContext targetContext)? Remember that consumers of the repo only see the outside. They'll wonder "am I going to insert a context? And into what?" This is bad, but the really bad thing is that the method has vastly unexpected side effects: it replaces the context's dbSet by targetContext.Set<T>. How can consumers even begin to expect that to happen? Together with the first point above I don't understand anything of these methods receiving DbContext as a parameter.

  2. No composability.

    OK, now if the above points wouldn't exist, you'd be left with a pretty regular repository. The only problem would then be that you can't use it to compose queries efficiently, because the "get" methods return IEnumerable, not IQueryable.

    Take this example:

      var results = from r1 in repo1.TableA.GetAll()
                    where r1.Id == someId
                    join r2 in repo2.GetAll() on r1.Id equals r2.TableAId
                    select new { ... };
    

    Returning IEnumerable has three major downsides:

    • No database filtering. As said above, GetAll() returns the full table content. Only after that the where is applied. An IQueryables would have pulled only one record from the database. Well, this point is alleviated somewhat by the Find method that accepts a predicate. But there's more.

    • Combined repositories execute separate queries. Both GetAll() methods run first and their results are joined in memory. Two IQueryables on the other hand, would create one expression that can be translated into one SQL call containing a JOIN and, obviously, the number of records from the second GetAll() would be limited. And this could not have been limited by using the Find with predicate, because there is no predicate. The join is a dynamic predicate, if you like.

    • No projection. The select new isn't translated back into a limited number of fields in the SQL SELECT statement.

tl;dr Together with the points mentioned in the other answer, very much is very wrong. Back to the drawing board. Or back to some generic repository implementation from the internet. They're all essentially identical.

\$\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.