patterncsharpMinor
Refactoring fat ASP.NET MVC Controller
Viewed 0 times
refactoringmvcfatcontrollernetasp
Problem
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 allRoles = _roleRepository.GetAsQueryable(where:null, includeProperties:"");
IQueryable 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.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.RoleNam
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 allRoles = _roleRepository.GetAsQueryable(where:null, includeProperties:"");
IQueryable 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.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.RoleNam
Solution
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 (
It all hinges one how much time you have and how much technical debt you want to incur :)
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 :)
Context
StackExchange Code Review Q#59041, answer score: 3
Revisions (0)
No revisions yet.