5
\$\begingroup\$

I'm new to this Repository pattern and have seen lot of repository pattern + UoW implementations all over the internet and I'm not able to reach to a conclusion as to which of them is correct . After going through many links I've been able to implement one .

Considering the following points in mind

  • It should satisfy SOLID principles
  • Be testable
  • Be independent of framework
  • Be independent of DB

Here is the code of the implementation

Generic IRepository

 public interface IRepository<T> where T : class
    {

        void Add(T entity);

        void Update(T entity);

        void Delete(T entity);

        T GetByKey(object id);

    }

RepositoryBase

public abstract class RepositoryBase<D, T> : IRepository<T> where T : class where D : BaseDbContext
{


    private D dataContext;
    private readonly IDbSet<T> dbSet;

    protected IDbFactory<D> DbFactory
    {
        get;
        private set;
    }

    protected D DbContext
    {
        get { return dataContext ?? (dataContext = DbFactory.Init()); }
    }


    protected RepositoryBase(IDbFactory<D> dbFactory)
    {

        DbFactory = dbFactory;
        dbSet = DbContext.Set<T>();

    }


    #region Implementation
    public virtual void Add(T entity)
    {
        dbSet.Add(entity);
    }

    public virtual void Update(T entity)
    {
        dbSet.Attach(entity);
        DbContext.Entry(entity).State = EntityState.Modified;
    }

    public virtual void Delete(T entity)
    {
        dbSet.Remove(entity);
    }


    public T GetByKey(object id)
    {
        return dbSet.Find(id);
    }


    #endregion

}

IUnitofWork , UnitOfWork

public interface IUnitOfWork<D> where D : BaseDbContext
    {
        void Commit();
    }



public class UnitOfWork<D> : IUnitOfWork<D> where D : BaseDbContext, new()
{
    private readonly IDbFactory<D> dbFactory;
    private D dbContext;

    public UnitOfWork(IDbFactory<D> dbFactory)
    {
        this.dbFactory = dbFactory;
    }

    public D DbContext
    {
        get { return dbContext ?? (dbContext = dbFactory.Init()); }
    }

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

IDBFactory ,DBFactory

public interface IDbFactory<D> where D : BaseDbContext
{
    D Init();
}




public class DbFactory<D> : Disposable, IDbFactory<D> where D : BaseDbContext, new()
        {
            D dbContext;
            public D Init()
            {
                return dbContext ?? (dbContext = new D());
            }
            protected override void DisposeCore()
            {
                if (dbContext != null)
                    dbContext.Dispose();
            }
        }

BaseDbContext

public abstract class BaseDbContext : DbContext
{
public BaseDbContext(string nameOrConnectionString) : base(nameOrConnectionString)
        {

        }

}

ProjectDbContext

 public partial class ProjectDbContext : BaseDbContext
    {
        public ProjectDbContext()
            : base("name=ProjectDbContext")
        {
            Database.SetInitializer<ProjectDbContext>(null);
        }


    }

EXAMPLE USAGE

Controller

 public class StudentsController : BaseController
    {

        private IStudentBusiness objstudentbusiness;
        public StudentsController(IStudentBusiness rstudentbusiness)
        {
            objstudentbusiness = rstudentbusiness;
        }


        public JsonResult LoadStudents()
        {

                var data = objstudentbusiness.ListStudents();
                var jsonResult = Json(data, JsonRequestBehavior.AllowGet);
                return jsonResult;

        }

    }

IStudentBAL,StudentBAL

 public interface IStudentBAL
    {
        void SaveStudent(StudentDto student);
        List<StudentDto> ListStudents();
    }


public class StudentBAL : BusinessBase, IStudentBAL
{

    private readonly IStudentRepository objStudentRepository;
    private readonly IUnitOfWork<ProjectDbContext> objIUnitOfWork;

    public StudentBAL(IStudentRepository rIStudentRepository, IUnitOfWork<ProjectDbContext> rIUnitOfWork)
    {
        try
        {
            objStudentRepository = rIStudentRepository;
            objIUnitOfWork = rIUnitOfWork;
        }
        catch (Exception ex)
        {

            Log.Error(ex);
        }

    }

    public List<StudentDto> ListStudents()
    {
        try
        {
            var tusrs = objStudentRepository.ListStudents() ?? new List<StudentDto>();
            return tusrs;
        }
        catch (Exception ex)
        {
            Log.Error(ex);

        }
        return new List<StudentDto>();
    }
}

IStudentRepository,StudentRepository

 public interface IStudentRepository
    {
        void SaveStudent(Student Student);
        StudentDto GetStudentByName(StudentDto Studentname);
        Student GetStudentByID(int Studentid);
        List<StudentDto> ListStudents();
    }

public class StudentRepository : RepositoryBase<ProjectDbContext, Student>, IStudentRepository
{
    public StudentRepository(IDbFactory<ProjectDbContext> dbFactory) : base(dbFactory)
    {
    }
    public List<StudentDto> ListStudents()
    {

            var students = (from t in DbContext.Students

                        select new StudentDto
                        {
                           // all the required properties
                        }).ToList();


            return students;

    }
}
  • Dependency Injection is done using AutoFac
  • Code for logging is omitted

I am asking if this seems like a good implementation or am I missing anything?

I would appreciate any feedback on my implementation you can offer regarding correctness, efficiency, and any suggestions. So here are my questions

  • Is this loosely coupled ?
  • Is it having any leaky abstraction and why ?
    • What needs to be done to switch from EF to another ORM such as Dapper or to simple ADO.net and how much effort would it require to implement the changes?
  • Is this pattern breaking any of the SOLID princples ,Law or Demeter or any Object Oriented laws?
  • Does this pattern have any code redundancies which are not required ?
  • How this architecture using EF would scale with a project containing more than 100+ domain entities and with each entity having at least more than 10 fields . Is it going to be maintenance nightmare later on?

-All criticisms greatly appreciated !!

EDIT : What needs to be done to switch from EF to another ORM such as Dapper or to simple ADO.net and how much effort would it require to implement the changes?

\$\endgroup\$
  • 2
    \$\begingroup\$ Be independent of framework -- well, you have private readonly IDbSet<T> dbSet;. Also see blog.ploeh.dk/2012/03/26/IQueryableTisTightCoupling \$\endgroup\$ – Gert Arnold Feb 14 '18 at 13:31
  • \$\begingroup\$ @GertArnold Can you please explain why it is dependent on the framework and also provide a general feedback over the overall implementation? \$\endgroup\$ – Codebadger Feb 14 '18 at 18:55
1
\$\begingroup\$

Considerations

It should satisfy SOLID principles

Keep in mind that SOLID isn't a binary state, where you either adhere to it or don't. SOLID is a guideline, the adherence to which is a spectrum.

Nothing is perfectly SOLID to the bone. You should only implement SOLID principles where reasonably valid. Implementing SOLID should be done not just for the sake of implementing SOLID.

This is just a sidenote to frame your expectations. The rest of the review accounts for SOLID where reasonable.

Be independent of framework
Be independent of DB

In order to be fully independent, you'd need to create separate libraries that only load the framework/DB library at runtime.
That creates more work than it solves, and is generally not a good approach (note specific cases, e.g. most mods for videogames).

However, we should indeed minimize code dependence on external libraries by using encapsulation.

A simple example here are your feet.

  • If you walk barefoot outside and then come inside, your feet are dirty. This is a problem.
  • So let's put on socks. Now, your socks get dirty when you walk outside, and your feet stay clean. However, the primary purpose of a sock is not to keep your feet clearn, it's to keep your feet warm. By using them to walk outside, you are essentially violating SRP with your socks. You don't want to take your socks off when you come in, because you don't want to have cold feet.
  • So let's put on shoes. Now, your shoes get dirty when walking outside, but your feet and socks stay clean. You can therefore transition from inside to outside without getting cold feet.

The important thing to take away here is that the bottom layer always gets dirty.

In your example, the Repository and UnitOfWork classes are that bottom layer. They interact with the dependency directly, just like how your shoes interact with the ground outside.
They exist specifically to ensure that your other layers (business logic, UI) don't get dirty; just like how your shoes enable your socks and feet to keep clean.

This again is just a sidenote, to frame your expectations when you say "independent". The code you posted is not independent itself (it relies on EF), but it does so specifically to ensure that the rest of your code can be independent.

Be testable

Not sure what you're specifically expecting here. I assume you're referring to the independency, i.e. that your tests don't handle EF directly.

What needs to be done to switch from EF to another ORM such as Dapper or to simple ADO.net and how much effort would it require to implement the changes?

This is easy enough to test. Remove the reference to EF, and see where you get errors.

The cost of implementing a new framework mostly depends on the cost of implementing said framework. Badly developed code might add some development time; but even the best code can't detract from the time needed to implement a certain framework.
To answer this question, simply try adding a Dapper repository and see how long it takes.

Is this pattern breaking any of the SOLID princples ,Law or Demeter or any Object Oriented laws?

Nothing sticks out. However, your classes are currently barebones and unlikely to get caught up on SOLID

SOLID mostly takes a pro-segregation stance. SOLID targets bulky code in order to argue where to draw needed separation.

Your current code has tiny classes, which means that it's unlikely to violate a SOLID rule since there's not enough pieces of logic to conflict with itself. (Liskov is an exception here and could still be violated, but nothing stands out).

If your classes get fleshed out more in the future, that is where SOLID has a better chance of raising issues.


Code review

IUnitOfWork

public interface IUnitOfWork<D> where D : BaseDbContext
{
    void Commit();
}

It's a bit weird to specify a generic parameter in an interface, but then not use it. It makes more sense to actually use it.

Secondly, D is a bad name. It's non-descriptive, and it doesn't follow naming conventions. Notice that your interface starts with I because that's the convention. Similarly, generic type parameters start with T.

public interface IUnitOfWork<TDbContext> where TDbContext : BaseDbContext
{
    TDbContext dbContext;
    void Commit();
}

Note that the naming convention helps with the reading. IUnitOfWork reads as "interface of the unit of work". Similarly, TDbContext reads as "type of the db context".

RepositoryBase

public abstract class RepositoryBase<D, T> : IRepository<T> where T : class where D : BaseDbContext

The same naming convention applies here:

public abstract class RepositoryBase<TDbContext, TEntity> : IRepository<TEntity> where TEntity : class where TDbContext : BaseDbContext

I'm conflicted about using a TDbContext. This suggests that any context is expected to provide access to any entity; which doesn't make sense to me. I can understand having separate db context classes if you're getting your data from multiple database (all via EF), but why would you ever have two db contexts that have access to the same entities?

This may be my failure to understand your intentions, so I won't draw a hard line here. But I suggest reconsidering this decision. Or if you could explain a valid use case for it in the comments? :)

BaseDbContext

Why is this needed? It provides nothing, it's literally jut passes its constructor to the unerlying base type. This class can be removed altogether. Change all existing references to directly reference DbContext.

Note that the existence of BaseDbContext doesn't actually provide any separation of dependency. Any code that needed to reference BaseDbContext would need to know what its base class (DbContext) was, and thus needed to depend on EF anyway.

If BaseDbContext had had a property of type DbContext, that would actually wrap the EF dependency. But they you'd be stuck having to make sure that BaseDbContext provides all needed properties/methods to interact with DbContext; which is a lot of work for no payoff, since your repositories are still relying on EF references anyway.

ProjectDbContext

Database.SetInitializer<ProjectDbContext>(null);

Why does this exist? Is this implemented specifically to prevent the default CreateDatabaseIfNotExists behavior? Because if so, you're only really creating a point of failure without getting any benefit from it.

Your version, where you don't create the database if it doesn't exist; therefore implicitly relies on the database existing (otherwise you end up with exceptions later on).
The default behavior, under the implicit assumption that the database already exists, would not do anything anyway (since the database already exists).

You're preventing behavior for a situation that you're assuming will never occur. I don't see the benefit of doing so.
Ignoring my personal distaste for excessive exceptions, I see no benefit gained from this line of code. The expected behavior (= the expectation that the database exists) will be just the same with or without this line. And that means that it can be removed.

IDBFactory ,DBFactory

I see no reason to use a factory here. Your factory is essentially a constructor call with no added logic. Since there's no added logic, there's no need for a factory.

Maybe I'm cynical, but this seems like one of those cases where something gets implmented because you like working with the thing (or your team lead tells you to implement it because he thinks it should always be used). These are just guesses based on experience.

My advice to counter that ideology: Existence must be justified. If you can't point out the benefit of having something, then there's no point to having it. Note that you don't need to argue the general benefit of factories, but rather the specific benefit for this application. Just because it was relevant in the previous project does not mean it is relevant in this one.

We all make the mistake of thinking "if you build it they will come". That's normal, especially if you've had to build it regularly in past projects. But you should re-evaluate its existence at a later stage too and see if it turns out to be redundant, which I think is the case for your factory here.

\$\endgroup\$
0
\$\begingroup\$

Can't address all your points but can give my opinions on several.

What needs to be done to switch from EF to MySQL?

You don't need to switch from EF to MySql, Entity Framework can be used to go against just about any data source. Simply (ok not always simply) point your EF to your MySql data source using the MySQL providers. With any luck it will be as simple as changing your connection string.

Does this pattern have any code redundancies which are not required ?

Some would argue that the repository pattern is redundant when used with Entity Framework since EF is already doing the things that the Repository Pattern and Unit Of Work paradigms call for. Mocks and testing might be the only case but even that can be done with a Dev data source and dummy data.

\$\endgroup\$
  • \$\begingroup\$ Instead of MySQL it is Dapper or any other ORM or simply ADO.net .I've edited my question. \$\endgroup\$ – Codebadger Feb 24 '18 at 5:31
  • \$\begingroup\$ it will be as simple as changing your connection string If I'm not mistaken, the provider should also be changed, which is slightly less obvious. Or does MySQL also use the default SqlProvider? \$\endgroup\$ – Flater May 25 '18 at 9:48

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.