I have relatively simple controller logic with ugly parts of object creation. Some of the objects are really huge and even though all the controller does is to return the object, it looks messy and hard to read. So I would like to ask how can I make such cases cleaner. Let me show you a few examples:

For instance, pay attention to the creation of viewmodel:

public async Task<ActionResult> Edit(Guid id)
{
    if (id.IsValidGuid())
    {
        // Get User Details
        var userTask = ApiGatewayService.GetUserDetail(id, AuthService.BearerToken);
        var nationalitiesTask = ApiGatewayService.GetNationalityList(
            new FilterParameters(), 
            AuthService.BearerToken
        );
        var countriesTask = ApiGatewayService.GetCountryList(
            new FilterParameters(), 
            AuthService.BearerToken
        );
        await Task.WhenAll(userTask, nationalitiesTask, countriesTask);
        var user = userTask.Result;
        var nationalities = nationalitiesTask.Result;
        var countries = countriesTask.Result;
        if (user is null)
            return NotFound();
        var userAddress = user.PrimaryAddress;
        var userHomePhone = user.Phones.FirstOrDefault(
            x => x.Type == PhoneType.LandLine
        );
        var userMobilePhone = user.Phones.FirstOrDefault(
            x => x.Type == PhoneType.Mobile
        );

        // View Model
        var viewModel = new EditUserViewModel
        {
            User = user,
            Id = user.Id,
            FirstName = user.FirstName,
            LastName = user.LastName,
            BirthDay = user.Detail?.BirthDay,
            Email = user.Email,
            Nationality = user.Detail?.Nationality?.Id,
            Title = user.Detail != null ? user.Detail.Title : TitleType.Mr,
            ProfilePhotoName = user.Detail?.ProfilePhotoName,
            MarketingOptin = user.Detail != null 
                ? user.Detail.MarketingOptin 
                : false,
            ChangePassword = false,
            Password = null,
            Address_City = userAddress?.City,
            Address_Country = userAddress?.Country.Id,
            Address_HouseName = userAddress?.HouseName,
            Address_HouseNumber = userAddress?.HouseNumber,
            Address_Line = userAddress?.AddressLine,
            Address_PostCode = userAddress?.PostCode,
            Address_StreetName = userAddress?.StreetName,
            Home_CountryCode = userHomePhone?.CountryCode?.ToString(),
            Home_PhoneNumber = userHomePhone?.PhoneNumber,
            Mobile_CountryCode = userMobilePhone?.CountryCode?.ToString(),
            Mobile_PhoneNumber = userMobilePhone?.PhoneNumber,
            CountriesList = countries.List,
            NationalitiesList = nationalities.List
        };
        return View(viewModel);
    }
    else
        return RedirectToAction("Index");
}

Or another one using LINQ with a huge select

public CompanyDetailModel GetCompanyDetails(Guid accountHolderId)
{
    CompanyDetailModel companyDetailModel = new CompanyDetailModel();

    var accountHolder = _dbContext.AccountHolders.FirstOrDefault(
        x => x.Id == accountHolderId
    );

    var companyId = accountHolder.ObjectId;
    var company = _dbContext.Companies.FirstOrDefault(x => x.Id == companyId);

    companyDetailModel.CompanyId = company.Id;
    companyDetailModel.Description = company.Description;
    companyDetailModel.Name = company.Name;
    companyDetailModel.RegistrationNumber = company.RegistrationNumber;
    companyDetailModel.VATNumber = company.VATNumber;

    var users = 
        from ah in _dbContext.AccountHolders
        join uc in _dbContext.UserCompanies 
            on ah.ObjectId 
            equals uc.CompanyId
        join u in _dbContext.Users 
            on uc.UserId 
            equals u.Id
        where ah.Id == accountHolderId
        select new CompanyUserModel
        {
            UserId = u.Id,
            CompanyId = (Guid)uc.CompanyId,
            FullName = u.FirstName + " " + u.LastName,
            Email = u.Email,
            PhoneNumber = u.PhoneNumber,
            UserCompanyRoleType = uc.Role
        }
    ;

    var invites = 
        from ci in _dbContext.UserCompanyInvites
        select new CompanyUserInviteModel
        {
            Id = ci.Id,
            CompanyId = ci.CompanyId,
            IsAccepted = ci.IsAccepted,
            UserEmail = ci.UserEmail,
            RoleType = ci.RoleType
        }
    ;

    companyDetailModel.CompanyUsers = users.ToList();
    companyDetailModel.CompanyUserInvites = invites.ToList();

    return companyDetailModel;
}

Is it sufficient to take the creation of objects to a private method, or is there a better way? Wouldn't it be really tedious to set up a mapper?

  • 2
    First write tests around those controller actions. When you have covered logic with tests - then unleash "refactoring hell" ;). – Fabio Nov 28 at 3:11
  • Is there reason for "flattening" the classes in ViewModels? All properties of users, addresses, phones, etc.. Why not just return the objects themselves and leave it up to view to do the null-propagation? – Euphoric Nov 28 at 5:53
  • 1
    @Euphoric Why would you make testing your software harder? Exposing your internal representation just causes coupling, and encourages migration of business logic into the display layer. – Kain0_0 Nov 28 at 6:00
  • I agree with Fabio here, get some solid tests around which fields source what data and how first. Then start picking at it. Also unless your IDE has unworldly powers in handling and debugging LINQ statements prefer to lean on easier to debug syntactic features like functions. Don't abandon LINQ (it has its uses) but the more complex the query the harder it will be to write tests for, and it will be harder to debug when something breaks. – Kain0_0 Nov 28 at 6:08
  • @Euphoric or at least an EditAddressViewModel and two EditPhoneViewModel members of EditUserViewModel – Caleth Nov 28 at 10:04
up vote 0 down vote accepted

Whether moving this logic into view models or data mappers, you have data mapping logic going on. Setting up a data mapper is no more tedious than the code already in your controllers. This appears to be more of a question about the aesthetics of code. It "looks" messy in the controller, and you want "clean" looking controllers. This can be accomplished in one of the following ways:

  1. Push this logic in the view models, where the view models hold references to your entities. This was already mentioned by Ewan.

    The nice thing about this approach is the logic for applying changes to your entities is housed in your view models. The disadvantage is the logic for applying changes to your entities is housed in your view models. Sometimes it's nice to keep the data in the view model completely separate from the entities so they are free to evolve differently, and for different reasons. This leads to:

  2. Use constructors in your view models to do the mapping, but the view models don't hold references to the entities and instead map directly to other properties.

    There is still coupling between view model and entity, but only in the constructors. Have another entity that needs to be mapped to the same view model? Add a constructor. This works well for small, simple view models.

    Which leads to the disadvantage of this approach: Constructor bloat. View models become 90% constructor code and 10% "stuff used by the view."

  3. Use a "view model factory" to create the view models based on entities.

    View models and entities are completely decoupled and are free to evolve separately. When either side changes, only the view model factory is impacted.

    This works well for complex view models, or cases where view models and/or entities change often, and for different reasons. The view model factory is where view models, entities and repositories meet.

    It's one more layer that insulates the other layers against change.

    This last bit can be the tipping point between solutions. When the construction of a view model becomes complex, using a view model factory implementing an interface, and then injecting this into your controller allows you to easily mock this complicated logic when unit testing your controllers.

The solution you choose will result in "cleaner" looking controller code, which seems to be your ultimate goal here. I've used a combination of numbers 2 and 3 before, with Number 2 being used for simple view models, and Number 3 being used for complex view models, or view models that represent an entire "page" or "screen" in the application.

With approaches 2 and 3, you will need an additional layer to apply changes to data in the view models to your entities. You can use the nebulous "service class" or go with a Use Case in clean architecture for this.

Either way, you have a need for code to map data from entities to view models, and then back again. This isn't going to change. All you can do is make it look prettier, and hopefully apply meaningful names to things.

Honestly I don't think it's too bad. The key thing is that the controller has a single responsibility.

If you want to make it a bit cleaner you can move the mapping the the ViewModel itself eg.

public class EditUserViewModel
{
    private User user;
    EditUserViewModel(User user) {this.user = user;}

    public bool MarketingOptin 
    {
        get 
        {
            return user.Detail != null ? user.Detail.MarketingOptin : false,
        }
        set {...}
    }
}

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.