How can I refactor my asp.net-mvc 5 controller to abstract away all these dependencies?

Question

I have an application that is written using c# on the top of ASP.NET MVC 5 framework. I installed and setup Unity.Mvc package which allows me to do dependency injection.

I have the following code in one of my controller. Although this code seems pretty clean to me, there are few things that are bothering me which I am trying to improve.

Looking at the following FlagsController you'll properly notice all these nasty dependencies that have to be injected into my controller. Second, I feel that my controller should only need to depend on FlagService, Flash, Mapper. The rest are dependencies that my ViewModels requires to make them presentable.

// Here is my current code 
public class FlagsController : Controller
{
    protected IProductService ProductService;
    protected IClientService ClientService;
    protected IUserPassport Passport;
    protected ITaskFlagService FlagService;
    protected IFlashManager Flash;
    protected IMapper Mapper;

    public FlagsController (
        IProductService productService,
        IClientService clientService,
        IUserPassport passport,
        ITaskFlagService flagService,
        IFlashManager flash,
        IMapper mapper)
    {
        ProductService = productService;
        ClientService = clientService;
        Passport = passport;
        FlagService = flagService;
        Flash = flash;
        Mapper = mapper;
    }

    public ActionResult Index(ListFlagsViewModel viewModel)
    {
        viewModel.Flags = FlagService.GetPagedRecords(viewModel, 25);

        viewModel.SetMenuItems(ProductService, ClientService, Passport);
        return View(viewModel);
    }

    public ActionResult Create()
    {
        var viewModel = new CreateFlagViewModel();
        viewModel.SetMenutItems(ClientService, Passport);

        return View(viewModel);
    }

    [HttpPost, ValidateAntiForgeryToken]
    public ActionResult Create(CreateFlagViewModel viewModel)
    {
        if (ModelState.IsValid)
        {
            try
            {
                FlagService.Add(viewModel);
                Flash.AddSuccess("New flag has been added.", 3);

                return RedirectToAction("Index");
            }
            catch (Exception e)
            {
                Flash.AddError(e.Message);
            }
        }

        viewModel.SetMenutItems(ClientService, Passport);

        return View(viewModel);
    }


    public ActionResult Edit(int? id)
    {
        var flag = FlagService.Get(id);
        if (flag == null)
        {
            return RedirectToAction("Index");
        }

        var viewModel = Mapper.Map<EditFlagViewModel>(flag);
        viewModel.SetMenutItems(ClientService, Passport, flag.ClientId, ProductService);

        return View(viewModel);
    }

    [HttpPost, ValidateAntiForgeryToken]
    public ActionResult Edit(EditFlagViewModel viewModel)
    {
        if (ModelState.IsValid)
        {
            var flag = FlagService.Get(viewModel.Id);
            if (flag == null)
            {
                return RedirectToAction("Index");
            }

            TaskFlag updatedFlag = Mapper.Map(viewModel, flag);
            FlagService.Update(updatedFlag);
            Flash.AddSuccess("Flag has been updated.", 3);

            return RedirectToAction("Index");
        }

        viewModel.SetMenutItems(ClientService, Passport, viewModel.Client.Id, ProductService);

        return View(viewModel);
    }

    public ActionResult Details(int? id)
    {
        var flag = FlagService.Get(id);
        if (flag == null)
        {
            return RedirectToAction("Index");
        }

        var viewModel = Mapper.Map<DisplayFlagViewModel>(flag);

        return View(viewModel);
    }
 }

Now, I am trying to keep my view-models strictly as a data-transfer-objects. But in order for my view models to transfer an entity-model into a view-ready html-form, it needs some services to pull data from the database for drop down menu or other things. Therefore, my view models is not going to have any business logic, it will however need to call different services in order to put all the pieces together to be view-ready. In order to explain this better, please have a look at my CreateFlagViewModel class listed below.

public class CreateFlagViewModel
{
    [Required, MaxLength(50)]
    public string Title { get; set; }

    public OptionalClientMenuViewModel Client { get; set; }
    public OptionalProductMenuViewModel Product { get; set; }

    [MaxLength(255), DataType(DataType.MultilineText)]
    public string Description { get; set; }

    public CreateFlagViewModel()
    {
        Client = new OptionalClientMenuViewModel();
        Product = new OptionalProductMenuViewModel();
    }

    public void SetMenutItems(IClientService clientService, IUserPassport passport)
    {
        Client.SetOptions(clientService, passport);
    }
}

You'll notice here that I am using sub-view-models into my viewmodel to allow me to reuse my code and also have EditorTemplate for each to eliminate duplicate code being written all over the place. Here is the sub-view-model aka OptionalClientMenuViewModel which is responsible of showing a list of clients that appears in the above view-model aka CreateFlagViewModel.

public class OptionalClientMenuViewModel
{
    public int? Id { get; set; }
    public IEnumerable<SelectListItem> Options { get; set; }

    public OptionalClientMenuViewModel()
    {
        Options = new List<SelectListItem>();
    }

    public void SetOptions(IClientService service, IUserPassport passport, bool isActive = true)
    {
        Options = service.GetClientItemForUser(passport.User, isActive);
    }
}

As you can see, the CreateFlagViewModel view-model class is pretty clean and simple. However it has that nasty SetMenuItems method which requires 2 dependence. These dependencies force any consumer "in this case the controller" to have to pass them along. I feel if I can somehow get rid of the dependencies out of the SetMenuItems method, my controller will get cleaned up, and my view-model will look cleaner.

Since I am using dependency injection, why can't I transfer my code to something like the following

First swapping the dependencies in my sub-view-model from the method into the constructor.

public class OptionalClientMenuViewModel
{
    protected IClientService ClientService;
    protected IUserPassport Passport;

    public int? Id { get; set; }
    public IEnumerable<SelectListItem> Options { get; set; }

    public OptionalClientMenuViewModel(IClientService clientService, IUserPassport passport)
    {
        Options = new List<SelectListItem>();
    }

    public void SetOptions(bool isActive = true)
    {
        Options = ClientService.GetClientItemForUser(Passport.User, isActive);
    }
}

Then create an interface to auto call the SetMenutItems() method every time a view will be rendered. public interface IHaveMenuSetter { void SetMenutItems(); }

Finally, implement the IHaveMenuSetter contract into my view-model and inject the dependencies into the constructor to make the SetMenuItems method parameter-less.

public class CreateFlagViewModel : IHaveMenuSetter
{
    [Required, MaxLength(50)]
    public string Title { get; set; }

    public OptionalClientMenuViewModel Client { get; set; }
    public OptionalProductMenuViewModel Product { get; set; }

    [MaxLength(255), DataType(DataType.MultilineText)]
    public string Description { get; set; }

    public CreateFlagViewModel(OptionalClientMenuViewModel client)
    {
        Client = client;
        Product = new OptionalProductMenuViewModel();
    }

    public void SetMenutItems()
    {
        Client.SetOptions();
    }
}

Finally, my controller will look something like this " assuming that the rest of the viewModels are refactored to the same code pattern."

public class FlagsController : Controller
{
    protected ITaskFlagService FlagService;
    protected IFlashManager Flash;
    protected IMapper Mapper;
    protected CreateFlagViewModel CreateViewModel;

    public FlagsController (
        ITaskFlagService flagService,
        IFlashManager flash,
        IMapper mapper, 
        CreateFlagViewModel createViewModel)
    {
        FlagService = flagService;
        Flash = flash;
        Mapper = mapper;
        CreateViewModel = createViewModel;
    }

    public ActionResult Index(ListFlagsViewModel viewModel)
    {
        viewModel.Flags = FlagService.GetPagedRecords(viewModel, 25);

        // This line could be even eliminated and auto called using `ActionFilterAttribute` by utilizing a new contract 
        viewModel.SetMenuItems();

        return View(viewModel);
    }

    public ActionResult Create()
    {
        // This line could be even eliminated and auto called using `ActionFilterAttribute` by utilizing a new contract
        CreateViewModel.SetMenutItems(); 

        return View(CreateViewModel);
    }

    [HttpPost, ValidateAntiForgeryToken]
    public ActionResult Create(CreateFlagViewModel viewModel)
    {
        if (ModelState.IsValid)
        {
            try
            {
                FlagService.Add(viewModel);
                Flash.AddSuccess("New flag has been added.", 3);

                return RedirectToAction("Index");
            }
            catch (Exception e)
            {
                Flash.AddError(e.Message);
            }
        }

        // This line could be even eliminated and auto called using `ActionFilterAttribute` by utilizing a new contract
        CreateViewModel.SetMenutItems(); 

        return View(viewModel);
    }


    public ActionResult Edit(int? id)
    {
        var flag = FlagService.Get(id);
        if (flag == null)
        {
            return RedirectToAction("Index");
        }

        var viewModel = Mapper.Map<EditFlagViewModel>(flag);

        // This line could be even eliminated and auto called using `ActionFilterAttribute` by utilizing a new contract
        viewModel.SetMenutItems(); 

        return View(viewModel);
    }

    [HttpPost, ValidateAntiForgeryToken]
    public ActionResult Edit(EditFlagViewModel viewModel)
    {
        if (ModelState.IsValid)
        {
            var flag = FlagService.Get(viewModel.Id);
            if (flag == null)
            {
                return RedirectToAction("Index");
            }

             TaskFlag updatedFlag = Mapper.Map(viewModel, flag);
            FlagService.Update(updatedFlag);
            Flash.AddSuccess("Flag has been updated.", 3);

            return RedirectToAction("Index");
        }

        // This line could be even eliminated and auto called using `ActionFilterAttribute` by utilizing a new contract
        viewModel.SetMenutItems(); 

        return View(viewModel);
    }

    public ActionResult Details(int? id)
    {
        var flag = FlagService.Get(id);
        if (flag == null)
        {
            return RedirectToAction("Index");
        }

        var viewModel = Mapper.Map<DisplayFlagViewModel>(flag);

        return View(viewModel);
    }
 }

Cleaner? Probably. But...the above code will give me an error as my view-models don't have a default constructor. This error occurs when asp.net-mvc DefaultModelBinder try to construct the view model during the HttpPost request.

In order for my setup to work, I would have to override the DefaultModelBinder so it resolves view-model from the IoC container instead of using the default constructor which is something I am hesitating to do.

Question

Does the state of my current code look acceptable? What about my proposed code change, doesn't the proposed code separates the concerns in a better matter? Is there a better route to clean this code up?

Answer 1

The major practical problem I see with your proposed solution is that CreateFlagViewModel needs to be passed as an action parameter, so MVC will need to be able to construct it, which it may have a hard time with considering it has a constructor parameter, and that constructor parameter has services as constructor parameters. There are probably workarounds available to make this work, but it's a sign that you're probably thinking in the wrong direction.

Now, I am trying to keep my view-models strictly as a data-transfer-objects.

Great idea. Let's try pushing your code even further in that direction by getting rid of methods like SetOptions.

But in order for my view models to transfer an entity-model into a view-ready html-form, it needs some services to pull data from the database for drop down menu or other things. Therefore, my view models is not going to have any business logic, it will however need to call different services in order to put all the pieces together to be view-ready.

Actually, I'd recommend taking that responsibility away from the ViewModel. How about creating viewmodel-mapping services that do the work defined in methods like SetOptions and SetMenuItems?

Instead of:

    viewModel.SetMenutItems(ClientService, Passport);

Use:

    FlagsModelMapper.SetMenutItems(viewModel);

That way, your service can abstract away the injection of ClientService and Passport. It stays independently unit-testable, as does your controller. And the ViewModel remains easy to deserialize.

This is in line with a pattern that I have found to be almost always the best pattern to follow, and which is espoused by experts like Mark Seemann. Split your classes into two categories: Services and Models. Models should be as dumb as possible, and if they have any logic at all it should only act on data internally available to the model itself. Services use constructor injection to inject other services they need. Never inject a model into a service, and never use a service in a model.