Thin controller design

Question

We have asked to keep the MVC controller as thin, so we decided to move all the service calling/business logic into the ViewModel class.

But is it good approach to use a ViewModel class to make a service call?

Does this follows the OOPS/SOLID principle?

public class EmployeeController:Controller
{

  private IEmployeeService EmpServ{get;set;}
  public EmployeeController(IEmployeeService service)
  {
   EmpServ=service;
  }

 public ActionResult GetEmployee(int Id)
 {
  EmployeeViewModel vm=new EmployeeViewModel(EmpServ);
  vm= vm.GetData(Id);
  //some 10 lines of code for validation logic, etc
  return View(vm);
 }

}

The view model class is calling the business service method

public class EmployeeViewModel
{
  public string Name{get;set;}
  //...etc

  private IEmployeeService EmpServ{get;set;}
  public EmployeeViewModel(IEmployeeService service)
  {
   EmpServ=service;
  }

  public EmployeeViewModel GetData(int Id)
  {
   Employee e= EmpServ.GetEmpDetails(id);
   EmployeeViewModel vm=ConvertEmpToViewModel(e);//another helper method
   return vm;
  }
}

Answer 1

We have asked to keep the MVC controller as thin, so we decided to move all the service calling/business logic into the ViewModel class.

The concept of the "thin" controller is a guideline to help you avoid having business logic in your presentation layer - it is not as simple as "having more than a little code in controller actions is bad."

In moving this code to your view models, you have effectively coupled them to your service layer without solving the real problem. The controller is responsible for mediating between your services and your presentation logic (views and view models) - that is the entire reason for its existence. Do not just shift code around arbitrarily because of some naive desire to reduce the amount of code in your controller actions.

If you find your controller has too much code in it (or, more importantly, that it has business logic in it), then what you need is an application facade layer that sits between your controller and your service layer, co-ordinates your services and provides a simpler interface for your controllers.

Alternatively, if the code you're trying to move is presentation-layer logic (such as model validation, construction of view models, processing of form values, mapping of view models to service-layer entities etc.), then it does belong in your controller - even if there are more than a couple of lines of it. You can still simplify things by breaking it up into helpers if it's beneficial to do so, but don't do this just because someone told you that your controller actions shouldn't have more than five lines of code in them.

Answer 2

Calling a constructor with the requirement that you have to call another method to initialize it seems like a recipe for disaster. What may make more sense is to have a static Create method:

public class EmployeeViewModel
{
    public static EmployeeViewModel Create(
        IEmployeeService service,
        int id)
    {
        var e = service.GetEmpDetails(id);
        var vm = ConvertEmpToViewModel(e);
        return vm;
    }
}

which you'd call like so:

var viewModel = EmployeeViewModel.Create(this.employeeService, id);

Answer 3

From asp.net tutorials:

Controllers: Classes that handle incoming browser requests, retrieve model data, and then specify view templates that return a response to the browser.

So, in order to keep your controllers thin, you might have the following:

public class MyController {

    private readonly IFooService fooService;

    public MyController(IFooService fooService) {
        this.fooService = fooService;
    }

    [HttpGet]
    public ActionResult Foo(int id) {
        var foo = fooService.Find(id);
        var model = new FooViewModel(foo);
        return View(model);
    }

    [HttpPost]
    public ActionResult CreateFoo(CreateFooModel model) {
        if (!ModelState.IsValid()) {
            return View(model);
        }

        var foo = model.ToFoo();
        fooService.Add(foo);

        // return Redirect() or so
    }
}

some 10 lines of code for validation logic, etc

You should not have validation logic in an action, especially in a "get" request. For "post" requests, the validation logic can be added on "create"-models (like CreateFooModel, in the example above) by implementing the IValidatableObject interface on the model in question.

The controllers will be thin enough if you do only this:

For GET requests:

• fetch the business object or more business objects from one or more application / business services
• map the business object / objects to a view model, preferably using just one line of code
• return a view - pass it the previously built view model.

For POST requests

• check if the model is valid (IValidatableObject should be implemented for the "create"-model in question). Supposing it is valid,
• instantiate a business object from the "create"-model, preferably in one line of code. Feel free to use a factory, a mapper or a simple function responsible with the conversion of the model to a business object
• pass the business object to one or more services, as required by your application
• return a redirect or whatever.

All in all, a GET action should generally have 3 lines of code, while a POST action might have 6-7 lines of code.

Also, you might have a look on:

• How we do MVC – View models (lostechies.com)
• https://stackoverflow.com/questions/25054416/viewmodels-cqrs-and-entities/25169023#25169023

Answer 4

Another option is to create an extension method for IEmployeeService:

public static class EmployeeServiceExtensions
{
    public static EmployeeViewModel CreateViewModel(
        this IEmployeeService service,
        int id)
    {
        var e = service.GetEmpDetails(id);
        var vm = ConvertEmpToViewModel(e);
        return vm;
    }
}

which you'd then call in your controller like so:

var viewModel = this.employeeService.CreateViewModel(id);