Skip to main content
Code Review

Return to Answer

grammar
Source Link
Adrian Iftode
Adrian Iftode
  • 371
  • 2
  • 17

So what did I just do here was to make the interface non-generic and move the generic type to the methods. Imagine now how easy is to do nextfollowing

    public class RemindersService
    {
       private readonly IRepositoryBase _repository;
       private readonly IUnitOfWork _unitOfWork;
       public RemindersService(IRepositoryBase repository, IUnitOfWork uow) 
       { 
          _repository = repository; 
          _uow = uow
       }
    
       public Reminder AddNewReminder(DateTime moment, string note, Guid receiver, string name)
       {
            var user = _repository.FindById(receiver);
            var reminder = new Reminder {ReceiverId = receiver, Moment = momoent, Note = note };
            _repository.Create(reminder);
            
            _uow.Save();
       }
    }

The main gain here is for very simplistic CRUD operations you don't need to create an interface and an implementation for you everyeach of entity fromin the DbContext.

Let's move next to the more specific repository. With the non-generic IRepositoryBaseIRepositoryBase it will look like this:

You have two options: either remove the interface inheritance or remove the the CreateReminder. If you remove the inheritance and keep the CreateXXX methods then you transmit the idea that the those XXXs are not handled as everything else by the repository, but the have a custom logic. I doubt that, since the repositories need to take care only for the store access. So remove these methods and let only the GetByName in the IReminderRepository. You might also decide to remove the interface inheritance and IRepositoryBase<Reminder> when you need those generic methods and IReminderRepository when you need the GetByName method.

SoLike I said you don't need to tie the UoW with the repositories interfaces. Follow this convention with the Repository suffix and you'll be fine to discover the repositories you need. OneOnce you have that, there is no need to transform the UoW into a ServiceLocator - Repositories FactoriesFactory.

Which brings the next: use an IoC for you dependencies. .Net Core? - built in. .Net Framework: a bunch of them. Still WebForms? There useUse Property Injection. So avoid newing your dependencies.

It's a repository, by defintion: Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects. Take a look at the IListIList API and you'll find better naming:

Use this API as EntityFramework provides the asynchronous way to handle the databases. You'll be scalable, your threads you will be free to do something else (like handling new requestrequests) and won't be blocked until the SQL Server decides to returns its results.

The FindAll returns an IList which means the DbContext will send the query to the SQL server and then will map the results to the Reminder objects. After that the ReminderRepository object will search this in memory collection for the first reminder. What if you have 100.000 reminders? Probably you don't want to do thisthat, but to generate the SQL that will have in it the WHERE clause. So avoid ToList().FirstOrDefault() or any indirection to that if you know you will need just a subset of these records/objects.

IList has methods like Remove, Clear etc. Most likely the repository clients won't need that and even if they use them, the outcome won't be the expected one. The FindAll should return something that with youthey can only iterate/read. So use IReadonlyCollection.

The FindByCondition has the drawback that you'll looselose some encapsulation and abstraction and you open the gate for code duplication. I personally like it, but if I see that I tend to copy the predicate in a new place, then II'll add a new method in the repository, like you just did with FindByName. People are usually lazy (including myself) and sooner or later you'll get in trouble with it.

So what did I just do here was make the interface non-generic and move the generic type to the methods. Imagine now how easy is to do next

public class RemindersService
{
   private readonly IRepositoryBase _repository;
   private readonly IUnitOfWork _unitOfWork;
   public RemindersService(IRepositoryBase repository, IUnitOfWork uow) { _repository = repository;}

   public Reminder AddNewReminder(DateTime moment, string note, Guid receiver, string name)
   {
        var user = _repository.FindById(receiver);
        var reminder = new Reminder {ReceiverId = receiver, Moment = momoent, Note = note };
        _repository.Create(reminder);
        
        _uow.Save();
   }
}

The main gain here is for very simplistic CRUD operations you don't need to create an interface and an implementation for you every entity from the DbContext.

Let's move next to the more specific repository. With the non-generic IRepositoryBase it will look like this:

You have two options: either remove the interface inheritance or remove the the CreateReminder. If you remove the inheritance and keep the CreateXXX methods then you transmit the idea that the those XXXs are not handled as everything else by the repository, but the have a custom logic. I doubt that, since the repositories need to take care only the store access. So remove these methods and let only the GetByName in the IReminderRepository. You might also decide to remove the interface inheritance and IRepositoryBase<Reminder> when you need those generic methods and IReminderRepository when you need the GetByName method.

So you don't need to tie the UoW with the repositories interfaces. Follow this convention with the Repository suffix and you'll be fine to discover the repositories you need. One you have that there is no need to transform the UoW into a ServiceLocator - Repositories Factories.

Which brings the next: use an IoC for you dependencies. .Net Core? - built in. .Net Framework: a bunch of them. Still WebForms? There use Property Injection. So avoid newing your dependencies.

It's a repository, by defintion: Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects. Take a look at the IList API and you'll find better naming:

Use this API as EntityFramework provides the asynchronous way to handle the databases. You'll be scalable, your threads you will be free to do something else (like handling new request) and won't be blocked until the SQL Server decides to returns its results.

The FindAll returns an IList which means the DbContext will send the query to the SQL server and then will map the results to the Reminder objects. After that the ReminderRepository object will search this in memory collection for the first reminder. What if you have 100.000 reminders? Probably you don't want to do this, but to generate the SQL that will have in it the WHERE clause. So avoid ToList().FirstOrDefault() or any indirection to that if you know you will need just a subset of these records/objects.

IList has methods like Remove, Clear etc. Most likely the repository clients won't need that and even if they use them, the outcome won't be the expected one. The FindAll should return something that with you can only iterate/read. So use IReadonlyCollection.

The FindByCondition has the drawback that you'll loose some encapsulation and abstraction and you open the gate for code duplication. I personally like it, but if I see that I tend to copy the predicate in a new place, then I add a new method in the repository, like you just did with FindByName. People are usually lazy (including myself) and sooner or later you'll get in trouble with it.

So what did I just do here was to make the interface non-generic and move the generic type to the methods. Imagine now how easy is to do following

    public class RemindersService
    {
       private readonly IRepositoryBase _repository;
       private readonly IUnitOfWork _unitOfWork;
       public RemindersService(IRepositoryBase repository, IUnitOfWork uow) 
       { 
          _repository = repository; 
          _uow = uow
       }
    
       public Reminder AddNewReminder(DateTime moment, string note, Guid receiver, string name)
       {
            var user = _repository.FindById(receiver);
            var reminder = new Reminder {ReceiverId = receiver, Moment = momoent, Note = note };
            _repository.Create(reminder);
            
            _uow.Save();
       }
    }

The main gain here is for very simplistic CRUD operations you don't need to create an interface and an implementation for each of entity in the DbContext.

Let's move next to the more specific repository. With the non-generic IRepositoryBase it will look like this:

You have two options: either remove the interface inheritance or remove the the CreateReminder. If you remove the inheritance and keep the CreateXXX methods then you transmit the idea that the those XXXs are not handled as everything else by the repository, but the have a custom logic. I doubt that, since the repositories need to take care only for the store access. So remove these methods and let only the GetByName in the IReminderRepository. You might also decide to remove the interface inheritance and IRepositoryBase<Reminder> when you need those generic methods and IReminderRepository when you need the GetByName method.

Like I said you don't need to tie the UoW with the repositories interfaces. Follow this convention with the Repository suffix and you'll be fine to discover the repositories you need. Once you have that, there is no need to transform the UoW into a ServiceLocator - Repositories Factory.

Which brings the next: use an IoC for you dependencies. .Net Core? - built in. .Net Framework: a bunch of them. Still WebForms? Use Property Injection.

It's a repository, by defintion: Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects. Take a look at the IList API and you'll find better naming:

Use this API as EntityFramework provides the asynchronous way to handle the databases. You'll be scalable, your threads you will be free to do something else (like handling new requests) and won't be blocked until the SQL Server decides to returns its results.

The FindAll returns an IList which means the DbContext will send the query to the SQL server and then will map the results to the Reminder objects. After that the ReminderRepository object will search this in memory collection for the first reminder. What if you have 100.000 reminders? Probably you don't want to do that, but to generate the SQL that will have in it the WHERE clause. So avoid ToList().FirstOrDefault() or any indirection to that if you know you will need just a subset of these records/objects.

IList has methods like Remove, Clear etc. Most likely the repository clients won't need that and even if they use them, the outcome won't be the expected one. The FindAll should return something that with they can only iterate/read. So use IReadonlyCollection.

The FindByCondition has the drawback that you'll lose some encapsulation and abstraction and you open the gate for code duplication. I personally like it, but if I see that I tend to copy the predicate in a new place, then I'll add a new method in the repository, like you just did with FindByName. People are usually lazy (including myself) and sooner or later you'll get in trouble with it.

Source Link
Adrian Iftode
Adrian Iftode
  • 371
  • 2
  • 17

Unneeded complexity

There is unnecessary complexity because of two reasons: the unit of work wants to be a repository locator and because the repositories are generic on the wrong side.

Remove the repositories references from the UoW and you will be fine. Let's see now a way to have a better generic way for your CRUD.

public interface IRepositoryBase
{
     IList<T> FindAll<T>();
     T FindByCondition<T>(Expression<Func<T, bool>> expression);
     void Create<T>(T entity);
     void Update<T>(T entity);
     void Delete<T>(T entity);
}

So what did I just do here was make the interface non-generic and move the generic type to the methods. Imagine now how easy is to do next

public class RemindersService
{
   private readonly IRepositoryBase _repository;
   private readonly IUnitOfWork _unitOfWork;
   public RemindersService(IRepositoryBase repository, IUnitOfWork uow) { _repository = repository;}

   public Reminder AddNewReminder(DateTime moment, string note, Guid receiver, string name)
   {
        var user = _repository.FindById(receiver);
        var reminder = new Reminder {ReceiverId = receiver, Moment = momoent, Note = note };
        _repository.Create(reminder);
        
        _uow.Save();
   }
}

The main gain here is for very simplistic CRUD operations you don't need to create an interface and an implementation for you every entity from the DbContext.

The downside of this approach is that now suddenly all the entities will have this generic way for the CRUD operations. Maybe you don't what that, but I consider this a non-issue in most of the applications. One potential issue is when you want to implement here and there let's say a soft-delete and the generic method does a hard delete. There are ways to overcome that.

Use the valuable API from DbContext

I used in the RemindersService.AddNewReminder a new method, FindById, that was not in the original IRepositoryBase definition. This can be easily added and dispatched to dbContext.Find(id). This method returns so you'll probably have to check for that in your Application Services.

Simplify abstractions, remove duplication

Let's move next to the more specific repository. With the non-generic IRepositoryBase it will look like this:

public interface IReminderRepository : IRepositoryBase
        {
        IList<Reminder> GetAllReminders();
        Reminder GetReminderById(Guid id);    
        Reminder GetReminderByName(string name);    
        void CreateReminder(Reminder reminder);    
        void UpdateReminder(Reminder reminder);    
        void DeleteReminder(Reminder reminder);    
    }

The first issue with this interface is the abstraction duplication: you have two ways to create a Reminder: CreateReminder and Create<Reminder> (or Create in the original). Same for Update, Delete and for FindAll. As an API consumer I would have a hard time to pick one because I will feel insecure about the fact I might have picked the wrong one. As an Implementation developer I will have to implement the same functionality twice or to just delegate to the base class.

You have two options: either remove the interface inheritance or remove the the CreateReminder. If you remove the inheritance and keep the CreateXXX methods then you transmit the idea that the those XXXs are not handled as everything else by the repository, but the have a custom logic. I doubt that, since the repositories need to take care only the store access. So remove these methods and let only the GetByName in the IReminderRepository. You might also decide to remove the interface inheritance and IRepositoryBase<Reminder> when you need those generic methods and IReminderRepository when you need the GetByName method.

So you don't need to tie the UoW with the repositories interfaces. Follow this convention with the Repository suffix and you'll be fine to discover the repositories you need. One you have that there is no need to transform the UoW into a ServiceLocator - Repositories Factories.

Avoid newing dependencies

Which brings the next: use an IoC for you dependencies. .Net Core? - built in. .Net Framework: a bunch of them. Still WebForms? There use Property Injection. So avoid newing your dependencies.

Avoid disposing what you didn't create

public class UnitOfWork : IUnitOfWork, IDisposable

Did UnitOfWork create the DbContext? Then don't dispose. Let the creator of the DbContext to decide when and if wants to dispose it.

Naming:

void Create(T entity);
void Update(T entity);
void Delete(T entity);

It's a repository, by defintion: Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects. Take a look at the IList API and you'll find better naming:

void Add(T entity);
void Remove(T entity);

A list doesn't have an Update method so frankly you don't need this method because of this:

{
  var reminder = _repository.FindById(reminderId);
  reminder.AddNote(note); // probably AddNote will do something like this.Notes.Add(note);
  uow.Save();
}

See? No Update method. The DbContext takes care of the changes made to the returned reminder and will commit them when uow.Save is called.

Async - Await

Use this API as EntityFramework provides the asynchronous way to handle the databases. You'll be scalable, your threads you will be free to do something else (like handling new request) and won't be blocked until the SQL Server decides to returns its results.

Big performance issue

public class ReminderRepository : RepositoryBase<Reminder>, IReminderRepository
{
    public ReminderRepository(DBContext dbContext)
    : base(dbContext)
    {
        _dbContext = dbContext;
    }
    //other methods removed
    public Reminder GetReminderByName(string name)
    {
        return FindAll()
            .OrderByDescending(r => r.Name)
            .FirstOrDefault(r => r.Name == name);

        //return FindByCondition(r => r.Name == name);
    }
}

The FindAll returns an IList which means the DbContext will send the query to the SQL server and then will map the results to the Reminder objects. After that the ReminderRepository object will search this in memory collection for the first reminder. What if you have 100.000 reminders? Probably you don't want to do this, but to generate the SQL that will have in it the WHERE clause. So avoid ToList().FirstOrDefault() or any indirection to that if you know you will need just a subset of these records/objects.

More about API

IList<T> FindAll();

IList has methods like Remove, Clear etc. Most likely the repository clients won't need that and even if they use them, the outcome won't be the expected one. The FindAll should return something that with you can only iterate/read. So use IReadonlyCollection.

How generic should be?

The FindByCondition has the drawback that you'll loose some encapsulation and abstraction and you open the gate for code duplication. I personally like it, but if I see that I tend to copy the predicate in a new place, then I add a new method in the repository, like you just did with FindByName. People are usually lazy (including myself) and sooner or later you'll get in trouble with it.