Skinny controller in ASP.NET MVC 4

Rails community are always inspire a lot of best ideas. I really love this community by the time. One of these is "Fat models and skinny controllers". I have spent a lot of time on ASP.NET MVC, and really I did some miss-takes, because I made the controller so fat. That make controller is really dirty and very hard to maintain in the future. It is violate seriously SRP principle and KISS as well. But how can we achieve that in ASP.NET MVC? That question is really clear after I read "Manning ASP.NET MVC 4 in Action". It is simple that we can separate it into ActionResult, and try to implementing logic and persistence data inside this. In last 2 years, I have read this from Jimmy Bogard blog, but in that time I never had a consideration about it. That's enough for talking now.

I just published a sample on ASP.NET MVC 4, implemented on Visual Studio 2012 RC at here. I used EF framework at here for implementing persistence layer, and also use 2 free templates from internet to make the UI for this sample.

In this sample, I try to implementing the simple magazine website that managing all articles, categories and news. It is not finished at all in this time, but no problems, because I just show you about how can we make the controller skinny. And I wanna hear more about your ideas.

The first thing, I am abstract the base ActionResult class like this:   

    public abstract class MyActionResult : ActionResult, IEnsureNotNull
    {
        public abstract void EnsureAllInjectInstanceNotNull();
    }
 
    public abstract class ActionResultBase<TController> : MyActionResult where TController : Controller
    {
        protected readonly Expression<Func<TController, ActionResult>> ViewNameExpression;
        protected readonly IExConfigurationManager ConfigurationManager;
 
        protected ActionResultBase (Expression<Func<TController, ActionResult>> expr)
            : this(DependencyResolver.Current.GetService<IExConfigurationManager>(), expr)
        {
        }
 
        protected ActionResultBase(
            IExConfigurationManager configurationManager,
            Expression<Func<TController, ActionResult>> expr)
        {
            Guard.ArgumentNotNull(expr, "ViewNameExpression");
            Guard.ArgumentNotNull(configurationManager, "ConfigurationManager");
 
            ViewNameExpression = expr;
            ConfigurationManager = configurationManager;
        }
 
        protected ViewResult GetViewResult<TViewModel>(TViewModel viewModel)
        {
            var m = (MethodCallExpression)ViewNameExpression.Body;
            if (m.Method.ReturnType != typeof(ActionResult))
            {
                throw new ArgumentException("ControllerAction method '" + m.Method.Name + "' does not return type ActionResult");
            }
 
            var result = new ViewResult
            {
                ViewName = m.Method.Name
            };
 
            result.ViewData.Model = viewModel;
 
            return result;
        }
 
        public override void ExecuteResult(ControllerContext context)
        {
            EnsureAllInjectInstanceNotNull();
        }
    }

I also have an interface for validation all inject objects. This interface make sure all inject objects that I inject using Autofac container are not null. The implementation of this as below

    public interface IEnsureNotNull
    {
        void EnsureAllInjectInstanceNotNull();
    }

Afterwards, I am just simple implementing the HomePageViewModelActionResult class like this

    public class HomePageViewModelActionResult<TController> : ActionResultBase<TController> where TController : Controller
    {
        #region variables & ctors
 
        private readonly ICategoryRepository _categoryRepository;
        private readonly IItemRepository _itemRepository;
 
        private readonly int _numOfPage;
 
        public HomePageViewModelActionResult(Expression<Func<TController, ActionResult>> viewNameExpression)
            : this(viewNameExpression,
                   DependencyResolver.Current.GetService<ICategoryRepository>(),
                   DependencyResolver.Current.GetService<IItemRepository>())
        {
        }
 
        public HomePageViewModelActionResult(
            Expression<Func<TController, ActionResult>> viewNameExpression,
            ICategoryRepository categoryRepository,
            IItemRepository itemRepository)
            : base(viewNameExpression)
        {
            _categoryRepository = categoryRepository;
            _itemRepository = itemRepository;
 
            _numOfPage = ConfigurationManager.GetAppConfigBy("NumOfPage").ToInteger();
        }
 
        #endregion
 
        #region implementation
 
        public override void ExecuteResult(ControllerContext context)
        {
            base.ExecuteResult(context);
 
            var cats = _categoryRepository.GetCategories();
 
            var mainViewModel = new HomePageViewModel();
            var headerViewModel = new HeaderViewModel();
            var footerViewModel = new FooterViewModel();
            var mainPageViewModel = new MainPageViewModel();
 
            headerViewModel.SiteTitle = "Magazine Website";
            if (cats != null && cats.Any())
            {
                headerViewModel.Categories = cats.ToList();
                footerViewModel.Categories = cats.ToList();
            }
 
            mainPageViewModel.LeftColumn = BindingDataForMainPageLeftColumnViewModel();
            mainPageViewModel.RightColumn = BindingDataForMainPageRightColumnViewModel();
 
            mainViewModel.Header = headerViewModel;
            mainViewModel.DashBoard = new DashboardViewModel();
            mainViewModel.Footer = footerViewModel;
            mainViewModel.MainPage = mainPageViewModel;
 
            GetViewResult(mainViewModel).ExecuteResult(context);
        }
 
        public override void EnsureAllInjectInstanceNotNull()
        {
            Guard.ArgumentNotNull(_categoryRepository, "CategoryRepository");
            Guard.ArgumentNotNull(_itemRepository, "ItemRepository");
            Guard.ArgumentMustMoreThanZero(_numOfPage, "NumOfPage");
        }
 
        #endregion
 
        #region private functions
 
        private MainPageRightColumnViewModel BindingDataForMainPageRightColumnViewModel()
        {
            var mainPageRightCol = new MainPageRightColumnViewModel();
 
            mainPageRightCol.LatestNews = _itemRepository.GetNewestItem(_numOfPage).ToList();
            mainPageRightCol.MostViews = _itemRepository.GetMostViews(_numOfPage).ToList();
 
            return mainPageRightCol;
        }
 
        private MainPageLeftColumnViewModel BindingDataForMainPageLeftColumnViewModel()
        {
            var mainPageLeftCol = new MainPageLeftColumnViewModel();
 
            var items = _itemRepository.GetNewestItem(_numOfPage);
 
            if (items != null && items.Any())
            {
                var firstItem = items.First();
 
                if (firstItem == null)
                    throw new NoNullAllowedException("First Item".ToNotNullErrorMessage());
 
                if (firstItem.ItemContent == null)
                    throw new NoNullAllowedException("First ItemContent".ToNotNullErrorMessage());
 
                mainPageLeftCol.FirstItem = firstItem;
 
                if (items.Count() > 1)
                {
                    mainPageLeftCol.RemainItems = items.Where(x => x.ItemContent != null && x.Id != mainPageLeftCol.FirstItem.Id).ToList();
                }
            }
 
            return mainPageLeftCol;
        }
 
        #endregion
    }

 Final step, I get into HomeController and add some line of codes like this

    [Authorize]
    public class HomeController : BaseController
    {
        [AllowAnonymous]
        public ActionResult Index()
        {
            return new HomePageViewModelActionResult<HomeController>(x=>x.Index());
        }
 
        [AllowAnonymous]
        public ActionResult Details(int id)
        {
            return new DetailsViewModelActionResult<HomeController>(x => x.Details(id), id);
        }
 
        [AllowAnonymous]
        public ActionResult Category(int id)
        {
            return new CategoryViewModelActionResult<HomeController>(x => x.Category(id), id);
        }
    }

As you see, the code in controller is really skinny, and all the logic I move to the custom ActionResult class. Some people said, it just move the code out of controller and put it to another class, so it is still hard to maintain. Look like it just move the complicate codes from one place to another place. But if you have a look and think it in details, you have to find out if you have code for processing all logic that related to HttpContext or something like this. You can do it on Controller, and try to delegating another logic  (such as processing business requirement, persistence data,...) to custom ActionResult class.

Tell me more your thinking, I am really willing to hear all of its from you guys.

All source codes can be find out at here.

19 Comments

  • Why not just inject appropriate services into the controller so it calls them to produce the result?

    Implementing it your way introduces just another place where the business goes through. Maintaining it would be very costly (your time and money).

    I really believe that the following code might be easier:

    [AllowAnonymous]
    public ActionResult Index()
    {
    // Let's assume the appropriateService implements IAppropriateService
    // interface that provides valid model for the view). Also it would
    // implement a repository pattern so the instance acts just like
    // a collection.
    return View(appropriateService);
    }

    Moreover -- most of the developers will read it and immediately understand. It's very clear what is the model and what is it's flow.

  • @Roland
    If I understand your sample correctly, I must say that sending instances of your services to the view itself just seems like poor separation of concerns.

    Your views are supposed to be dumb, not interact with business logics or data access. Putting in interface in there does not change anything.

    The Controller should call the service, then return the result of that call to the view, which is responsible for rendering the data given.

    My 2 cents anyway

  • ThangChung, this is really very helpful, You are the real King of helping others.

  • I guess I'm kind of lost too. My models dont include much other than structure and I tend to do most of my business in its own layer. My controls handle very little. They mostly delegate responsabilty to the business layer and pass the respones back to the view.
    If you ask me, if you are doing more than a few lines of code in your controller and that code isn't delegating, your probably doing it wrong

  • First, this solution invokes more work than it solves.

    Second, I think you should all read the following article on MVC vs PAC. I think you'll be highly surprised to find that why you know as MVC, just isn't.

    http://www.garfieldtech.com/blog/mvc-vs-pac

  • Very nice an informative article

  • @RagingPenguin: I read the link about MVC vs. PAC. His description of MVC is just not correct. Certainly not in terms of what MVC has become today with regard to the web. The meaning of MVC has evolved over time. Early MVC systems (prior to the Web) had the controller handling things like click events. In the case of the web, it is more of a front controller model. But the idea: "If the display component does not have direct, random-access, pull-access to the data store, then it is not MVC" is just plainly false. In fact MVC is a UI pattern and does not imply persistence at all. The model in MVC is oftentimes just a view model that is available to present data to the view, but that data may or may not correlate to a business model or database model.

    I'm afraid the author's criticism of others "demonstrates a lack of understanding about MVC" should be applied to himself.

  • @Trevor

    In part, you're correct. In many of my discussions with others, I've made the same points you have: that the very definition of MVC has evolved to compensate for the stateless (generally speaking) nature of the web. However, it raises some decent questions. Considering the was already a name (and acronym) for what we call MVC today (PAC), why mangle the definitin of another already existing pattern? If our definition of an apple evolves over time to be closer to that of the orange, why not simple call them oranges? If we insist on still calling them apples, then don't oranges then become apples? That's the main point I was trying to make. I honestly don't care what people certain frameworks, I just want them to really think about what's going on.

  • This looks like very clumsy.

    How do i do model validation?

    Will it be effective for unit testing?

    i would rathar have a service in controller say viewfactory, and let it do the stuff for me.

  • This is one of the highly impressive article, i was not aware about the topic before but after reading this post, i am feeling very excited. Very well said article. I must say it will going to help lots of professionals.

  • mus5Od Thanks for sharing, this is a fantastic blog post.Thanks Again.

  • If you want to grow your know-how just keep visiting
    this site and be updated with the newest information posted here.

  • SxAEYk Appreciate you sharing, great blog article.Thanks Again. Great.

  • ra461G Im thankful for the blog. Awesome.

  • y16C11 Awesome blog.Much thanks again. Great.

  • Over engineered? I don't see anything wrong with using Ioc, Service layer and some builder classes for your viewModels. Some devs are never happy and think that the more complex they can make their code the better. With this solution you are missing out on some core MVC framework features.. Just keep it simple, no need to over complicate things like this, where does it all end?

  • Thanks, I’ve just been searching for information approximately this subject for a while and yours is the greatest I have discovered so far. But, what in regards to the bottom line? Are you certain in regards to the source?

  • I think this is missing the point of skinny controller, fat model. The idea behind the concept was to take all of the business logic and have the model handle that. Additionally, data access should be abstracted to a repository or service(a scope in the model in the case of rails). The goal is not to just remove as much code from the controller as possible. It is to ensure that the controller is only responsible for moving data from the repository or service to the view. This strategy just unnecessarily complicates the code.

    An effect skinny controller example would be

    public ActionResult UpdateItem(Foo input)
    {

    var oldFoo = _fooService.GetFoo(input.id);
    UpdateModel(oldFoo);
    _fooService.SaveChanges();
    return View(foo);
    }

    In that example, the controller method calls the service, binds the view input to model, and then has the service save changes. It is straight forward and very low complexity.

  • First thanks for this tip.
    But I have a question, when user do update, and there are invalid input, how to redirect to the previous page and keep the input of user.

Comments have been disabled for this content.