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.

Answer 1

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.