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.


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.