Static method vs. instance method in typical 3-tier business layer

Question

Currently, I am building a typical 3-tier web app with ASP.NET MVC. I have set it up with dependency injection (Autofac) like below:

public class UserController : BaseController
{
    private readonly IUserService _userService;
    public UserController(IUserService userService)
    {
        this._userService = userService;
    }
}

public class IUserService
{
    void InsertUser(User user);
    void UpdateUser(User user);
    void DeleteUser(User user);
}
public class UserService : IUserService
{
    private readonly IRepository<User> _userRepository;
    public UserService(IRepository<User> userRepository)
    {
        this._userRepository = userRepository;
    }
    public void InsertUser(User user)
    {
        _userRepository.Insert(user);
    }
    public void UpdateUser(User user)
    {
        _userRepository.Update(user);
    }
    public void DeleteUser(User user)
    {
        _userRepository.Delete(user);
    }
}

Repository is a typical generic repository using EF.

public interface IRepository<T> where T : BaseEntity
{
    void Insert(T entity);
    void Update(T entity);
    void Delete(T entity);
}

The problem is my app has a lot of entities, and for each entity I have to replicate the above code for CRUD operations in service layer. For example: With entity "Role", I have "InsertRole", "UpdateRole", "DeleteRole"... and many more for other entities. So I try to refactor to remove duplicate code by extracting CRUD operations to a STATIC CLASS "CommonService" with STATIC METHODs like below:

public static class CommonService
{
    public static void Insert<T>(T entity) where T : BaseEntity
    {
        var repository = EngineContext.Current.Resolve<IRepository<T>>();
        repository.Insert(entity);
    }
    public static void Update<T>(T entity) where T : BaseEntity
    {
        var repository = EngineContext.Current.Resolve<IRepository<T>>();
        repository.Update(entity);
    }
    public static void Delete<T>(T entity) where T : BaseEntity
    {
        var repository = EngineContext.Current.Resolve<IRepository<T>>();
        repository.Delete(entity);
    }
}

With this class, I will remove duplicate code in service for CRUD operations. In Controller, I just call CommonService.Insert(user);... It's really good with me now. I still have another service methods as normal and no duplication for CRUD. But I'm wondering if there's any downside with this approach except unit testing (I won't unit test for CRUD). Is there any problem with memory management and concurrency handling in web environment (ASP.NET MVC)? I haven't implemented concurrency mechanism for data handling with EF yet (simultaneous update an entity...)

Thanks in advance! MillDol.

Seems like very [leaky abstraction](https://en.wikipedia.org/wiki/Leaky_abstraction) to me. I would implement service specific functionality and not a blanket set of CRUD operations here. I think that once you start development you will realize you probably do not need or want that to begin with. Keep the CRUD in the Repository Layer and Business Logic that makes use of this in your Service layer (BLL). — Igor, Feb 07 '16 at 16:53
@Igor: yes, I think this is too abstraction. But if the CRUD is only in Repository Layer, then the Controller will depend on both Business and Repository? I work on many projects for enterprise and see that the business is just forwarding call to Repository. Is there any architecture that solved this problem? — MillDol, Feb 07 '16 at 17:11

score 2 · Accepted Answer · answered Feb 07 '16 at 16:30

If you decide to keep that static implementation, create a interface and proxy class that uses this and you can still unit test implementations that use it. You don't want to throw away unit testing.

public interface ICommonService<T>
{
    void Insert<T>(T entity);
    void Update<T>(T entity);
    void Delete<T>(T entity);
}

and implement a simple proxy type that implements ICommonService<T> and forwards calls to the static class. You can then depend upon ICommonService<T> and mock it out later is tests just like you could before.

I wouldn't have the static class. I don't recognize EngineContext.Current, but it looks like the service locator pattern. That's generally discouraged because it hides what you're depending on from obvious inspection.

You could still have a common interface like ICommonService<T>, then implement the proxy to depend upon an IRepository<T>,

public class CommonService<T> : ICommonService<T> where T : BaseEntity
{
    private readonly IRepository<T> repository;

    public CommonService(IRepository<T> repository)
    {  
        if (repository == null) throw new ArgumentNullException(nameof(repository)); 
        this.repository = repository;
    }

    // and other methods
}

then you can have you controller depend on the ICommonService, and you don't have to have static method calls behind the scenes.

got your idea. I will give it a try. just wondering if my framework is too abstraction and have another way to remove it too. — MillDol, Feb 07 '16 at 17:13

score 0 · Answer 2 · answered Dec 28 '20 at 17:14

I know this is very old, but just came across this and am wondering why don't you just take that Static implementation and make it an abstract class - something like a BaseService. That way you can have a single implementation of your methods, to remove all the duplicate code you mentioned, but still if any particular entity needs any special treatment you can handle it in each specific service implementation.

Static method vs. instance method in typical 3-tier business layer

2 Answers2