I started refactoring an ASP.Net 5 web application which uses MVC 6 and Entity Framework 7 when I was wondering about some points. My controllers currently use the DbContext
implementation via dependency injection to fill the view models and return them for rendering with the view. Like:
public class UserController : Controller
{
[FromServices]
public MyContext myContext { get; set; }
//route which renders all users
public IActionResult Index()
{
var users = myContext.User.OrderBy(u => u.Name);
List<UserIndexViewModel> v = new List<UserIndexViewModel>();
foreach (var item in users)
{
v.Add(new UserIndexViewModel { Name = item.Name, City = item.City, DateOfBirth = item.DateOfBirth });
}
return View(v);
}
//route to edit user
public async Task<ActionResult> Edit(int id)
{
User user = await FindUserAsync(id);
UserEditViewModel v = new UserEditViewModel { Name = user.Name, City = user.City };
return View(v);
}
[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Update(int id, UserEditViewModel userModel)
{
User user = await FindUserAsync(id);
try
{
user.Name = userModel.Name;
user.City = userModel.City;
myContext.User.Attach(user);
myContext.Entry(user).State = EntityState.Modified;
await myContext.SaveChangesAsync();
return RedirectToAction("Index");
}
catch (Exception)
{
ModelState.AddModelError(string.Empty, "Unable to save changes.");
}
return View(userModel);
}
private Task<User> FindUserAsync(int id)
{
return myContext.User.SingleOrDefaultAsync(u => u.UserId == id);
}
}
I did some research and I found some blog posts (like this) which asks to keep controllers clean. Ok.. why not?
I started creating a kind of view model builder to put the logic out of the controller methods over to the view model builder. In the above linked article is a hint to create for each view model a own view-model-builder class - which makes sense in my eyes.
For the model User
exist two views and thus two view models (UserIndexViewModel
and UserEditViewModel
). When I create the related view-model-builder classes they should derivate from the same (abstract) class because it could be that both child classes needs a helper method (like FindUserAsync()
- it is not the case in my example but just imagine). So I would have a construct like the following:
public interface IViewModelBuilder<TController, TViewModel>
{
TViewModel Build(TController controller, TViewModel viewModel);
Task<TViewModel > BuildAsync(TController controller, TViewModel viewModel);
}
public abstract class UserViewModelBuilder<TViewModel> : IViewModelBuilder<UserController, TViewModel> { ... }
public class UserIndexViewModelBuilder : SiteViewModelBuilder<UserIndexViewModel> { ... }
public class UserEditViewModelBuilder : SiteViewModelBuilder<UserEditViewModel> { ... }
This mentioned function which is needed by multiple view model builders of one model should be implemented in the abstract class (UserViewModelBuilder
in my case), right?
I did it like this:
public abstract class UserViewModelBuilder<TViewModel> : IViewModelBuilder<UserController, TViewModel>
{
[FromServices]
public MyContext myContext { get; set; }
public abstract TViewModel Build(UserController controller, TViewModel viewModel);
public abstract Task<TViewModel> BuildAsync(UserController controller, TViewModel viewModel);
public Task<User> FindUserAsync(int id)
{
return myContext.User.SingleOrDefaultAsync(u => u.UserId == id);
}
}
So with this abstract class I can create the implementation of UserIndexViewModelBuilder
and UserEditViewModelBuilder
classes as well as for example UserDeleteViewModelBuilder
and UserCreateViewModelBuilder
classes in future...
Now the question:
- Is this a right approach to separate the logic from the controller? If yes, do I need a kind of factory for all view model builders which can be accessed via DI in all my controllers? If it is the way which is kind of best practice for MVC applications. Is there something different to the mentioned guide which can/should be used in MVC 6 apps?
- Is the abstract class the right place to call the
DbContext
implementation via DI? It feels not good for me. - Some other points which I missed? Some if-statements for checking response were removed from the UserController snipped for better readability.
Thank you! :-)