1
\$\begingroup\$

At work we use almost always the same pattern EF, DAL and BLL. I wish to try something new. This is the project structure:

Visual Studio Project

And here a code sample with only Users entity:

public interface IRepository : IUsersRepository
{
    void Save();
}

public partial class Repository : IRepository
{
    private DatabaseEntities _context;

    private DatabaseEntities Context
    {
        get
        {
            if (_context == null)
            {
                _context = new DatabaseEntities();
            }
            return _context;
        }
    }

    public void Save()
    {
        Context.SaveChanges();
    }
}

public interface IUsersRepository
{
    IList<User> GetUsers();

    User GetUserById(int id);

    void AddUser(User entity);

    void UpdateUser(User entity);

    void DeleteUser(User entity);
}

public partial class Repository : IUsersRepository
{
    public IList<User> GetUsers()
    {
        return Context.Users
            .OrderBy(x => x.Name)
            .ToList();
    }

    public User GetUserById(int id)
    {
        return Context.Users
            .Where(x => x.UserID == id)
            .FirstOrDefault();
    }

    public void AddUser(User entity)
    {
        Context.Users.Add(entity);
    }

    public void UpdateUser(User entity)
    {
        Context.Users.Attach(entity);
        Context.Entry(entity).State = EntityState.Modified;
    }

    public void DeleteUser(User entity)
    {
        Context.Users.Remove(entity);
    }
}

public interface IBusinessLogic : IUserBusinessLogic { }

public partial class BusinessLogic : IBusinessLogic
{
    private IRepository repository = null;
    private IRepository Repository
    {
        get
        {
            if (repository == null)
                repository = new Repository();
            return repository;
        }
    }
}

public interface IUserBusinessLogic
{
    IList<User> GetUsers();

    User GetUserById(int id);

    void AddUser(User entity);

    void UpdateUser(User entity);

    void DeleteUser(User entity);
}

public partial class BusinessLogic : IUserBusinessLogic
{
    public IList<User> GetUsers()
    {
        return Repository.GetUsers();
    }

    public User GetUserById(int id)
    {
        return Repository.GetUserById(id);
    }

    public void AddUser(User entity)
    {
        Repository.AddUser(entity);
        Repository.Save();
    }

    public void UpdateUser(User entity)
    {
        Repository.UpdateUser(entity);
        Repository.Save();
    }

    public void DeleteUser(User entity)
    {
        Repository.DeleteUser(entity);
        Repository.Save();
    }
}

As you can see the code is really linear and simply. all classes are partial classes of Repository or BusinessLogic that inherit by IRepository and IBusinessLogic, these interfaces inherit all other entities interfaces then with one instance of Repository or BusinessLogic object can call methods of all entities. Main BusinessLogic initializes an Repository object and usually in web application project there is an instance of BusinessLogic.

Most of the projects are simple CRUD operations so I'm wondering if there is some way to generalize these methods considering that the operations are always the same. Change only the entity and the property where query.

\$\endgroup\$
  • \$\begingroup\$ Is this a working code ? I can see some members not being implemented from the inherited interfaces. \$\endgroup\$ – Denis Jan 6 '17 at 21:42
  • \$\begingroup\$ Sorry, I made some error editing the code with Notepad++ for post it here. It has been taken from working code but I needed to do some change, anyway I hope that the logic is understandable. \$\endgroup\$ – Yiyi Chen Jan 6 '17 at 21:49
  • 1
    \$\begingroup\$ OK, I checked the code in Visual Studio and now it's working, sorry. \$\endgroup\$ – Yiyi Chen Jan 6 '17 at 21:54
4
\$\begingroup\$

You have a real interface pollution there. They do not provide any useful abstraction. Keep only one of them that you make generic. That's all you need.

public interface IRepository<T>
{
    IList<T> GetAll();

    T GetById(int id);

    void Add(T entity);

    void Update(T entity);

    void Delete(T entity);

    void Save();
}

private DatabaseEntities Context
{
    get
    {
        if (_context == null)
        {
            _context = new DatabaseEntities();
        }
        return _context;
    }
}

Don't create the context like this. It needs to be disposed as soon as possible. You queries should create the context for themselfes:

public IList<User> GetAll()
{
    using(var context = new DatabaseEntities())
    {
        return context.Users
            .OrderBy(x => x.Name)
            .ToList();
    }
}

Lastly don't do it like this at all. Create a single repository where you put all the queries you need and you're good. There's no need to create a repository for each entity type. I bet most of the APIs you'll never use anyway. Entity Framework is already a repository so creating a CRUD wrapper for all entities is just a waste of time and having repositories just for the sake of having implemented the CRUD repository pattern is pointless.

This repository virus spreads like a disease infecting all projects. Everone thinks they need a repository but they don't. What they need is just a bunch of queries to get the job done.

\$\endgroup\$
  • \$\begingroup\$ In our code database context never disposes... It's not an advantage this solution where context, repository and business logic classes are initialized only once in the application? About Generic Repository, each entity repository inherit from the generic once and, for example in MVC project, each controller initializes its relative repository? Is it this what do you mean? \$\endgroup\$ – Yiyi Chen Jan 7 '17 at 11:38
  • \$\begingroup\$ The few times I've worked with entity framework, I also never disposed the Context.. \$\endgroup\$ – Denis Jan 7 '17 at 13:40
  • 1
    \$\begingroup\$ @denis because most of the time you don't reach the limits so you were lucky :-) fire a select that gets a few hundered rows over and over agian from a large table in a long running application (days or weeks) and you'll learn to dispose it too (the hard way) ;-P in a web-app it's not so important because its lifetime is very short anyway but the OP did not mention the type of the application. \$\endgroup\$ – t3chb0t Jan 7 '17 at 13:45
  • \$\begingroup\$ Is possibile to generalize the entities repository implementations too? They do the same work, changes only the entity and properties in the queries. Is it a good practice? \$\endgroup\$ – Yiyi Chen Jan 8 '17 at 17:34
2
\$\begingroup\$

I can't say much about your code as it's rather short and it consists primarily of interfaces which can't be improved all that much, but there are few things that catch my eye :

public partial class Repository : IUsersRepository
public partial class Repository : IRepository

public interface IRepository : IUsersRepository

As you can see IRepository already inherits IUsersRepository so there is no need to inherit both of the interfaces, you can simply inherit the derived one, namely IRepository. Same goes for your BusinessLogic class.

null coalescing operator

You can shorten your code a bit by using the null coalescing operator at few places :

private IRepository Repository
{
    get
    {
        if (repository == null)
            repository = new Repository();
        return repository;
    }
}

private DatabaseEntities Context
{
    get
    {
        if (_context == null)
        {
            _context = new DatabaseEntities();
        }
        return _context;
    }
}

Can become :

private IRepository Repository
{
    get { return repository ?? (repository = new Repository()); }
}

private DatabaseEntities Context
{
    get { return _context ?? (_context = new DatabaseEntities()); }
}

Or using expression bodies :

private IRepository Repository => repository ?? (repository = new Repository());

private DatabaseEntities Context => _context ?? (_context = new DatabaseEntities());

FirstOrDefault with predicate

public User GetUserById(int id)
{
    return Context.Users
        .Where(x => x.UserID == id)
        .FirstOrDefault();
}

You can simply call FirstOrDefault and avoid the .Where() call, because FirstOrDefault also accepts predicates, this goes for probably all of the LINQ extension methods :

public User GetUserById(int id)
{
    return Context.Users
        .FirstOrDefault(x => x.UserID == id);
}

Few remarks :

Why do you need IBusinessLogic ? It's empty anyway, unless you are not showing some of your code there you better remove it.

Your partial modifier seems redundant to me, your classes look more like a normal class, but I won't stress on that as it might count as a personal preference or company convention.

Usually when you modify the database you should save the changes but you are not doing that in your AddUser, UpdateUser, DeleteUser methods.

IRepository sounds like the base interface rather than IUsersRepository which is a lot more concrete. It's like putting a IDog as a base interface instead of IAnimal.

\$\endgroup\$
  • \$\begingroup\$ Thanks for null coalescing operator and FirstOrDefault with predicate suggestions, I never thought about it. At a certain point you only do what other wants... About BusinessLogic, I don't implemented it in example code, usually it's used for operations such as log in a database table. For example: "User ABC added XYZ record". And after both operations (database modify and log) Save() has been called so it's a single EntityFramework transaction and if something fails no modifications has been done. \$\endgroup\$ – Yiyi Chen Jan 7 '17 at 10:23
  • \$\begingroup\$ It's not my code, I never designed a repository my self. Who "suggested" me this one affirm that using partial modifier allows to have all query methods in a single class and that divide them in different classes and interfances (for each entity) make code more readable and maintainable. \$\endgroup\$ – Yiyi Chen Jan 7 '17 at 10:31

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.