Autofac Parameterized Instantiation (Custom Factory) vs Resolve class by new

Question

I have minimized the problem which is: I have a class that returns customer remainig limit. I initilaize this class just in Mvc Controllers(driver project) not in a service or other project.

  public class AccountLimit
    {
        private readonly IMoneyService _moneyService;
        private readonly ILimitService _limitService;
        private readonly long _customerId;
        
        public AccountLimit(long customerId,IMoneyService moneyService,ILimitService limitService)
        {
            _moneyService = moneyService;
            _limitService = limitService;
            _customerId = customerId;
        }

        private decimal withdrawalSum { get { return _moneyService.GetTotalWithdrawal(_customerId); } }

        private decimal depositSum { get { return _moneyService.GetTotalDeposit(_customerId); } }

        private decimal depositLimit { get { return _limitService.GetDeposlitLimit(_customerId); } }

        private decimal withDrawalLimit { get { return _limitService.GetwithdrawalLimit(_customerId); } }

        public decimal RemainingDeposit{ get { return depositLimit - depositSum; } }

        public decimal RemainingWithdrawal { get { return withdrawalLimit - withdrawalSum; } }
    }

Some controllers just needs RemaningDeposit, some of them just needs RemaningWithdrawal some of them needs both. One of them:

 [Authorize]
    public class DepositController : Controller
    {
        private readonly IMoneyService _moneyService;
        private readonly ILimitService _limitService;
        private readonly ICustomerService _customerService;

        public DepositController(ICustomerService customerService, IMoneyService moneyService, ILimitService limitService)
        {
            _moneyService = moneyService;
            _limitService = limitService;
            _customerService = customerService;
        }

        public ActionResult GetDepositLimit()
        {
            var customer = _customerService.Find(User.Identity.Name);
            var accountLimit = new AccountLimit(customer.Id,_moneyService,_limitService);
            return View(accountLimit.RemainingDeposit);
        }
    }

Then I thought I can use Parameterized Instantiation which is auto-generated factory by Autofac to pass strongly-typed parameters to the resolution function.

Then my class will have interface public class AccountLimit : IAccountLimit And my deposit controller will be like this:

 [Authorize]
    public class DepositController : Controller
    {
        private readonly Func<long, IAccountLimit> _accountLimit;
        private readonly ICustomerService _customerService;

        public DepositController(ICustomerService customerService, Func<long, IAccountLimit> accountLimit)
        {
            _accountLimit = accountLimit;
            _customerService = customerService;
        }

        public ActionResult GetDepositLimit()
        {
            var customer = _customerService.Find(User.Identity.Name);
            var myAccountLimit = _accountLimit(customer.Id);
            return View(myAccountLimit.RemainingDeposit);
        }
    }

Without Parameterized Instantiation I have new AccountLimit in my code. My controllers have reference of IMoney and ILimitService.

With Parameterized Instantiation I don't have new in my code I don't have references. But If other developers don't use Autofac how do they resolve or initilaize this Func<long, IAccountLimit> _accountLimit ?

Question: I am confused. Is Parameterized Instantiation neccessary here ? I can test my code without it because I can mock customer, money and limit services. I'm just using this class on controllers, so is Parameterized Instantiation overstatement on here ?

Simply: Is my first code without Parameterized Instantiation is anti pattern ? Is it ok or should I use Parameterized Instantiation or custom Factory ?

I couldn't find a satisfying answer for these questions by myself. That's why I need your help.

Thanks for your help!

Answer 1

I feel like you're going about this in an unneccessarily complex way. The only dependencies I see on AccountLimit are IMoneyService and ILimitService.

CustomerId should be passed into each of the AccountLimit's "properties" (make them methods accepting CustomerId as arguments). Nowhere does your AccountLimit mainting state information about CustomerId other than orchestrating between the 2 dependent services, so it doesn't need to store it in it's state.

I'd implement it like:

    public class AccountLimit
    {
        private readonly IMoneyService _moneyService;
        private readonly ILimitService _limitService;

        public AccountLimit(IMoneyService moneyService,ILimitService limitService)
        {
            _moneyService = moneyService;
            _limitService = limitService;
        }

        private decimal WithdrawalSum(long customerId) {  return _moneyService.GetTotalWithdrawal(customerId); }

        private decimal WithDrawalLimit(long customerId) {  return _moneyService.withDrawalLimit(customerId); }

        public decimal RemainingWithdrawal(long customerId) {  return withdrawalLimit(customerId) - withdrawalSum(customerId); } 
    }

and in your controller:

    [Authorize]
    public class DepositController : Controller
    {
        private readonly IAccountLimit _accountLimit;
        private readonly ICustomerService _customerService;

        public DepositController(ICustomerService customerService, IAccountLimit accountLimit)
        {
            _accountLimit = accountLimit;
            _customerService = customerService;
        }

        public ActionResult GetWithdrawalLimit()
        {
            var customer = _customerService.Find(User.Identity.Name);

            return View(_accountLimit.RemainingWithdrawal(customer.Id));
        }
    }

Answer 2

Assuming your point is to simplify your controller and make it easier to test, rather than using parameterized instantiation, I'd probably go with a factory interface and a simple implementation backing it. From a separation of concerns standpoint, I've always felt like a lot of that sort of business logic shouldn't be in a controller anyway.

You started with this constructor:

public DepositController(
  ICustomerService customerService,
  IMoneyService moneyService,
  ILimitService limitService)

You then refactored in what (rightly) appears to be an effort to remove the controller's dependency on an IMoneyService because the controller doesn't really need to know about instantiating AccountLimit objects and, after all, the point is to get to the limit part of things. The only reason for the IMoneyService was for the AccountLimit after all.

public DepositController(
  ICustomerService customerService,
  Func<long, IAccountLimit> accountLimit)

However, from the snippet it appears you could do the same thing with the ICustomerService. What if you had this?

public DepositController(Func<string, IAccountLimit> accountLimit)

Then you could just pass in the username and get right to the meat of the thing.

var limit = _accountLimit(User.Identity.Name);

But then you mentioned you want to test things, which is reasonable, and be able to easily mock things in the constructor, so I'd say a simple solution is to refactor to a small provider interface.

public interface IUserLimitProvider
{
  IAccountLimit GetLimitForUser(string userName);
}

The implementation would be simple enough, taken right from your original controller code.

public class UserLimitProvider : IUserLimitProvider
{
  public UserLimitProvider(
    ICustomerService customerService,
    IMoneyService moneyService,
    ILimitService limitService)
  {
    // store these in private fields
  }

  public IAccountLimit GetLimitForUser(string userName)
  {
    var customer = _customerService.Find(userName);
    return new AccountLimit(customer.Id, _moneyService, _limitService);
  }
}

I'm guessing you probably want more robust error handling in that factory than what's shown, so having it in a separate method like this rather than a lambda in Autofac registrations will make it easier to test.

If the whole point of the controller is to be that provider then there's no point in refactoring to the Func<long, IAccountLimit> accountLimit version of the constructor in the first place. If the controller's purpose in life is to orchestrate the connection of those three services - ICustomerService, IMoneyService, and ILimitService - then let it take in the dependencies it needs to do so and call it a day.

However, I'm guessing based on my own experience that the controller really isn't where you want to do that, so if it was me... I'd go with the little provider interface.

Bonus: It appears that the whole point of the ICustomerService in this case is to map the user name into a user ID. I'm guessing you do that in several places. You may want to consider using ClaimsPrincipal and adding a claim to the principal with the user's ID right when they sign in. Then instead of using User.Identity.Name You could get the user's ID claim off the principal directly - skip all the lookups - and go right to the services with the appropriate ID. It'd be a decent perf gain, especially if that mapping is always a DB call. Plus it'd remove the need for the ICustomerService dependency in a few places, simplifying additional questions you'll encounter around this sort of challenge.