Where do you like to put MVC view model data transformation logic?

Question

Here is the scenario, I need to load up a view model object from a several domain objects that are being returned from multiple web service service calls. The code that transforms the domain model objects into the digestible view model object is a bit of fairly complex code. The three places that I have thought about putting it are:

1. Internal methods inside of the controller to load up an instance view model.
2. Static get method or property on the view model class itself that returns a loaded instance of view model.
3. An altogether separate builder or helper class that has a Static get method, property, or overloaded constructor that returns the loaded instance of view model.

To be clear, I don't want to use AutoMapper, or tools like it. From a best practices standpoint I'd like to know where this logic should go, and why.

EDIT

So here is what I have so far, this does give me "skinny" controller logic and separation of concerns. How can I make it better though?

    // ** Controller **
    public ActionResult Default()
    {

        var viewModel = MyViewModelBuilder.BuildViewModel(MarketType.Spot);

        return View(SpotViewUrl, viewModel);
    }

    // ** Builder **
    // Lives in MVC project under ViewModelBuilders folder
    public class MyViewModelBuilder
    {
        public static ChartsModel BuildViewModel(MarketType rateMarket)
        {
            var result = new ChartsModel
            {
                RateMarket = rateMarket,
                DateRange = new DateRange()
            };
            LoadGroupedRateLists(result, rateMarket);
            LoadCoorespondingRates(result);
            return result;
        }

        private static void LoadGroupedRateLists(ChartsModel model, RateMarket rateMarket)
        {
            var rateHistSvc = new RateHistoryService(RatesPrincipal.Current.Session.Token);
            var serviceResult = (rateMarket == RateMarket.Spot)
                                                                    ? rateHistSvc.GetSpotNationalRateHistory()
                                                                    : rateHistSvc.GetContractNationalRateHistory();

            // Break lists apart by category, and re-sort and trim.
            model.Cat1Rates = CategorizeTrimAndSort("cat1", false, serviceResult);
            model.Cat2Rates = CategorizeTrimAndSort("cat2", true, serviceResult);
            model.Cat3Rates = CategorizeTrimAndSort("cat3", false, serviceResult);
            model.Cat4Rates = CategorizeTrimAndSort("cat4", true, serviceResult);
            model.Cat5Rates = CategorizeTrimAndSort("cat5", false, serviceResult);
            model.Cat6Rates = CategorizeTrimAndSort("cat6", true, serviceResult);


            // Get Date range from results.
            var sortedRateMonths = serviceResultNational
                                    .Select(rate => rate.YearMonth)
                                    .OrderBy(ym => ym.Year)
                                    .ThenBy(ym => ym.Month);

            model.DateRange.Start = sortedRateMonths.First();
            model.DateRange.End = sortedRateMonths.Last();
        }   

        ...
    }

Answer 1

1 or 3, not 2. Provided that if you do #3, you do not actually let the static method do the web service calls, just let it do the mapping. Domain object(s) in, viewmodel(s) out. Prefer extension method to overloaded constructor, if the object doesn't need to track state, there's no benefit to making it non-static.

Why?

As soon as you put a logic method on the model, it ceases to be a POCO. Best practice is to treat viewmodels like boring data buckets as much as possible. Some people also try to do the mapping in a viewmodel constructor which is not a good idea once you get into any kind of complexity in the mapping.

If you only need to do the mapping in one controller, you can put it in a sub-routine. Keep in mind if you want to test the sub in isolation and keep it internal, your project will have to have InternalsVisibleTo your test project.

Update

After looking at your code, I am kind of inclined to agree with @C Sharper that this belongs neither in the controller, viewmodel, nor helper class / method. Composing this ChartsModel is very interesting code, and contains a lot of business logic. It really should be in a separate layer. Your controller should call down into that layer and delegate all of this interesting and important code to another abstraction. That abstraction should then return a domain object, as @C Sharper said. Whether you use that domain object as your viewmodel, or DTO it into a different viewmodel, is up to you. Here's how something like that might look like:

public class MyController : Controller
{
    private readonly IComposeChartData _chartDataComposer;

    public MyController(IComposeChartData chartDataComposer)
    {
        _chartDataComposer = chartDataComposer;
    }

    public ActionResult Default()
    {
        var chartComposition = new ChartCompositionSettings
        {
            MarketType = MarketType.Spot,
            Token = RatesPrincipal.Current.Session.Token,
        };
        var chartData = _chartDataComposer.ComposeChartData(chartComposition);
        var chartModel = Mapper.Map<ChartsModel>(chartData);
        return View(SpotViewUrl, chartModel);
    }
}

That is a nice lean controller body. The abstraction might then look something like this:

public class ChartDataComposer : IComposeChartData
{
    public ChartData ComposeChartData(ChartCompositionSettings settings)
    {
        // all of the interesting code goes here
    }
}

This way, your viewmodel does not need to move out into a separate layer, but you do need to create a similar object (ChartData) in that layer. The interface decouples your controller from the data it needs, and the object it returns is cohesive with your presentation data (viewmodel).

I guess I don't really see this code as business logic, but more as presentation logic. Why do you see it as business logic?

Think of your RateHistoryService class as a supplier. You receive raw materials from it, and transform those raw materials into something different, creating value in the process. This is what businesses do.

In this case, the chart visualizations are the value you are providing. Otherwise, your customers would have to sift through the raw data, trim it, categorize it, sort it, group it, etc., themselves before they could create similar charts.

I probably should have explained this earlier, but the service calls are to our own business layer already, and return domain layer business objects. It seems weird to me to have more than one business layer.

Your business layer can have its own internal layering. In this case, you can create a RateChartingService that uses the RateHistoryService to return a RateChartingResult busines object. Then map that to a ChartsModel (or like I said before, use it directly as your viewmodel).

Answer 2

I would say don't do it in your Controller. Controllers should be as "skinny" as possible. I would do it like this.

Your "Data Layer" would assign the Domain objects its properties and values. Then your subsequent Layer call it "Business Layer" will transfer your Domain Object into your ViewModel. And you will simply pass the view model to your controller without having the Controller handle any of that logic.

Separation is very important. Domain Objects should stay out of the controller, and the controllers should only care about View Models.