Refactoring fat ASP.NET MVC Controller

Question

I have just began porting an old project to ASP.NET MVC . In the end, I'll have a lot of controller methods like the one below, called by AJAX requests done by JQGrid objects in the pages:

AJAX request:

/UserRole/RolesByUserGridData?userId=2&_search=false&nd=1407171194811&rows=10&page=1&sidx=RoleName&sord=asc

Controller code:

public ActionResult GetRolesByUserGridData(MvcJqGrid.GridSettings gridSettings, int userId)
    {
        IQueryable<Role> allRoles = _roleRepository.GetAsQueryable(where:null, includeProperties:"");
        IQueryable<UserRole> userRolesByUser = _repository.GetAsQueryable(where: c => c.UserId == userId, includeProperties: "Role");

        var gridItems = from role in allRoles
                        join userRole in userRolesByUser on role.Id equals userRole.RoleId into loj
                        from item in loj.DefaultIfEmpty()   // LEFT OUTER JOIN equivalent 
                        select new UserRoleByUserViewModel
                        {
                            IsRoleAssociatedWithUser = (item.UserId == userId),
                            RoleName = role.Name
                        };

        SortOrder sortOrder = gridSettings.SortOrder != "desc" ? SortOrder.Asc : SortOrder.Desc;

        var pagedOrderedItems = PagingHelper<UserRoleByUserViewModel>.GetAsOrderedPagedList(
            gridItems, sortOrder, gridSettings.SortColumn,
            gridSettings.PageIndex, gridSettings.PageSize);

        var jsonData = new
        {
            total = pagedOrderedItems.TotalItemCount / gridSettings.PageSize + 1,
            page = gridSettings.PageIndex,
            records = pagedOrderedItems.TotalItemCount,
            rows = (
                from c in pagedOrderedItems
                select new
                {
                    id = "",
                    cell = new[]
                    {
                        "Edit", 
                        "Details",
                        c.IsRoleAssociatedWithUser.ToString(),
                        c.RoleName
                    }
                }).ToArray()
        };

        return Json(jsonData, JsonRequestBehavior.AllowGet);
    }

My goal is to return IEnumerable from repositories and have fairly dumb controllers.

How would you suggest refactoring this?

Ideally, the code in my controller method should be:

public ActionResult GetRolesByUserGridData(MvcJqGrid.GridSettings gs, int userId)
{
   var json = _userRoleService
      .GetRolesByUser()
      .ToOrderedList(“Id”, “Asc”)
      .ToPagedList(gs.PageIndex, gs.PageSize)
      .ToJsonData(); 
   return Json(jsonData, JsonRequestBehavior.AllowGet);
}

where ToOrderedList(), ToPagedList() and ToJsonData() would be extension methods to IQueryable<>, placed outside the Web project.

But it's still not clear to me where should the ViewModels live in this scenario:

A. Have them in the Service layer, which is in a different project - but then they lose all the logic related to Data Annotation, or I need to reference MVC in the Service layer, which seems weird.

B. Have them in the Web project - but why shouldn't the service be aware of the ViewModel and only retrieve the data that the ViewModel needs? Is it really necessary to create a new DTO object only to pass data from the service to the controller?

Answer 1

There's a few issues I can see with this code.

The repositories are are sitting in the presentation layer. A big no no. It's software 101, don't place data access in your presentation layer, and not good to put business logic in the presentation layer.

Those _roleRepository calls need to go in to a RoleRetrievalService or something similar. That centralise the business logic.

Your repositories should to sit behind a service layer which handles all of the calls to the DAL. Your service layer should contain business logic and data transformation which turns those DAL entities in to models which can be consumed by the presentation layer.

The query which does the left join should go in the service layer with the parameters required to execute the query being passed in as parameters. You could also pass in the sort options as well.

That last statement which builds the results (jsonData) is exactly what should be produced by your service layer.

It all hinges one how much time you have and how much technical debt you want to incur :)