2

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:

  1. 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?
  2. Is the abstract class the right place to call the DbContext implementation via DI? It feels not good for me.
  3. Some other points which I missed? Some if-statements for checking response were removed from the UserController snipped for better readability.

Thank you! :-)

maxarndt
  • 560
  • 7
  • 16
  • Perhaps better on codereview http://codereview.stackexchange.com/help/on-topic – Jasen Dec 08 '15 at 19:37
  • I think my post "is not about a particular piece of code and instead is a generally applicable question" (quote from codereview.stackexchange.com) – maxarndt Dec 08 '15 at 19:47
  • 2
    I do believe in clean code but I would take this article with a grain of salt. it looks like it's complicating things when there is no reason to. you want to map objects? use auto mapper package instead of writing interfaces and factories. it can be good for learning but not as a good practice – Avi Dec 08 '15 at 20:30

0 Answers0