Thin controller design 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;
  }
}
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.