Refactoring ViewModel and mapping from multiple sources

Question

I need to refactor and tidy the following controller below, it's my first MVC application. I've successfully used AutoMapper as I began refactor controllers but I am not sure how to approach a scenario where the data for the ViewModel comes from multiple service calls. And that scenario seems to often present itself in slightly heavier view models where I have all kind of information to present.

Calling multiple mapping calls on top of the same view model seemed counter-intuitive and it feels like I am missing some good practice or common approach to the situation. Or maybe I am wrong and it's commonly used approach.

0) Thought of generally using AutoMapper somehow but as I said I cannot find a proper way to implement it, I'd like to keep the controller relatively thin but I also don't want to have too much unneeded/overkill abstraction.

1) Thought of having some build method on the view model and to pass needed data.

2) Thought of having a model between my database entities and the view model but I am not sure how to execute that either, I think I seen somewhere by examples that people return "Models" from service calls that contain complex data and then map those Models to the ViewModel with Automapper, instead of mapping service calls directly into the ViewModel like others are doing.

3) Saw people use a facade of sorts in one example and thought of that too.

It's just my first project under MVC and I would like to learn a robust general approach that is considered well written and good practice so I can use it to fully refactor this and continue to improve from there.

        BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));
        var cutomData = JsonConvert.DeserializeObject<NopOrderData>(BKQEventData.custom.data.ToString());

        var model = new TaskEventViewModel();
        if (BKQEventData != null)
        {
            AuthenticateRequest(BKQEventData.xcm.clid, BKQEventData.xcm.opid);

            model.PlacesList = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name");
            model.ServicesList = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name");
            model.ContractorsList = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName");
            model.StatusList = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name");

            model.TaskEvent = new TaskEvent();
            model.TaskEvent.Title = string.Empty;
            model.TaskEvent.BKQCustomData = BKQEventData.custom.data.ToString();
            model.TaskEvent.BKQCustomDataType = BKQEventData.custom.type.ToString();
            model.TaskEvent.BKQClientId = BKQEventData.xcm.clid;
            model.TaskEvent.BKQOperatorId = BKQEventData.xcm.opid;
            model.TaskEvent.BKQInfProductId = BKQEventData.xcm.iprodid;
            model.TaskEvent.BKQEventHistoryId = BKQEventData.xcm.evnhid;
            model.TaskEvent.BKQEventTypeId = BKQEventData.xcm.evntype;
            model.TaskEvent.BKQEventCaseId = BKQEventData.xcm.caseid;
            model.TaskEvent.BKQQuota = BKQEventData.xcm.quotaid;
            model.TaskEvent.BKQLangId = BKQEventData.xcm.langid;
            model.TaskEvent.BKQContactId = BKQEventData.xcm.contid;
            model.TaskEvent.CreatedBy = BKQEventData.xcm.opid;
            model.TaskEvent.ModifiedBy = BKQEventData.xcm.opid;
            model.BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid);
        }
        else
        {
            return RedirectToActionPermanent("Login", "Login");
        }

Answer 1

When you have multiple sources of data you can still combine them into a single ViewModel in the Controller. If you have really "heavy" ViewModels that's an indication that you're displaying a lot of data at once, maybe you can find a way to break that up. Sometimes you can't due to requirements, but if you can that'd make both your life and your user's life easier (they wouldn't have to see/think about as much either).

In this case, your ViewModel doesn't look like it has that many properties: PlacesList, ServicesList, ContractorsList, StatusList, TaskEvent, and BKQContact. ViewModels for standard contact forms can have more properties. For example: FirstName, LastName, MiddleInitial, AddressLine1, AddressLine2, City, State, ZipCode, PhoneNumber. I don't think anyone would bat an eye over those properties in a ViewModel.

The first thing I'd do is reverse the if condition and move it right under the first line, where you're initializing BKQEventData. var cutomData... relies on BKQCustomData, but it's using it before the null check; if BKQEventData is null it looks like you'll get a Null Pointer Exception (NPE). Reversing the conditional is a matter of preference, but it prevents some indentation in some cases. So:

BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));

if (BKQEventData == null)
{
    return RedirectToActionPermanent("Login", "Login");
}

The next big thing I see is how many lines you have dedicated to setting up the TaskEvent. I'm guessing the TaskEvent is something you've defined in your project. With the exception of the Title property, all thirteen properties are derived from BKQEventData. You could add a method (or maybe a constructor) to TaskEvent that accepts a BKQEventCustomData and returns a TaskEvent. Or, if you don't have access to TaskEvent, you could make an extension method:

public static class TaskEventExtensions
{
    public static void InitalizeWithBKQEventCustomData(this TaskEvent taskEvent, BKQEventCustomData customData)
    {
        taskEvent.BKQCustomData = customData.custom.data.ToString();
        taskEvent.BKQCustomDataType = customData.custom.type.ToString();
        taskEvent.BKQClientId = customData.xcm.clid;
        taskEvent.BKQOperatorId = customData.xcm.opid;
        taskEvent.BKQInfProductId = customData.xcm.iprodid;
        taskEvent.BKQEventHistoryId = customData.xcm.evnhid;
        taskEvent.BKQEventTypeId = customData.xcm.evntype;
        taskEvent.BKQEventCaseId = customData.xcm.caseid;
        taskEvent.BKQQuota = customData.xcm.quotaid;
        taskEvent.BKQLangId = customData.xcm.langid;
        taskEvent.BKQContactId = customData.xcm.contid;
        taskEvent.CreatedBy = customData.xcm.opid;
        taskEvent.ModifiedBy = customData.xcm.opid;
    }
}

Going by this snippet I don't think that cutomData is used anywhere; if you're not using it, get rid of it. Dead code is a little messy and can make things confusing.

You could also use object initialization notiation to set up the majority of the ViewModel. It's a bit of a matter of preference, but I like it. Something along the lines of:

var model = new TaskEventViewModel {
    BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid),
    ContractorsList = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName"),
    PlacesList = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name"),
    ServicesList = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name"),
    StatusList = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name"),
    TaskEvent = new TaskEvent {
        Title = string.Empty
    }
};

model.TaskEvent.InitalizeWithBKQEventCustomData(BKQEventData);

Although I would probably pull the SelectLists out into their own variables. They're a little long, but you'd have to figure out how to best name/break them up.

Something else that I'd look at offhand is what happens when AuthenticateRequest fails? I have no way to tell in this snippet, but it doesn't look like it's handled.

So taking a pass at this using the extension method I mentioned earlier, this is what I got. It's a bit of a readability improvement to me. Again, the biggest thing that still stands out at me is how long those new SelectList lines are; I'd definitely break those into their own lines with a variable describing what exactly they are. Maybe something like:

BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));

if (BKQEventData == null)
{
    return RedirectToActionPermanent("Login", "Login");
}

AuthenticateRequest(BKQEventData.xcm.clid, BKQEventData.xcm.opid);

var clientsContractors = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName");
var activePlaces = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name");
var activeServices = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name");
var clientStatuses = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name");

var model = new TaskEventViewModel {
    BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid),
    ContractorsList = clientsContractors,
    PlacesList = activePlaces,
    ServicesList = activeServices,
    StatusList = clientStatuses,
    TaskEvent = new TaskEvent {
        Title = string.Empty
    }
};

model.TaskEvent.InitalizeWithBKQEventCustomData(BKQEventData);