No blog post can cover ASP.NET MVC controller best practices in one go. So this is the first post in a series about ASP.NET MVC controller best practices.

In this post we’re looking at why your ASP.NET MVC controllers should be ‘skinny’.

The code used in this post can be downloaded here.

I’ve heard Jeffrey Palermo say if you can’t see a ASP.NET MVC action method on a screen without having to scroll, you have a problem. In most cases the problem is the controller has multiple responsibilities.

The code in this post is adapted from a code sample I saw in an ASP.NET MVC book.

The controller looks like this

using System;
using System.Linq;
using System.Web.Mvc;
using TestingMvcControllers.Interfaces;
using TestingMvcControllers.Models;

namespace TestingMvcControllers.Controllers
{
    public class AnimalsController : Controller
    {
        public int PageSize = 4;
        private readonly IAnimalsRepository _animalsRepository;

        public AnimalsController(IAnimalsRepository animalsRepository)
        {
            _animalsRepository = animalsRepository;
        }

        public ViewResult List(string category, int page)
        {
            var animalsInCategory = (category == null)
                                        ? _animalsRepository.Animals
                                        : _animalsRepository.Animals.Where(x => x.Category == category);

            int numberOfAnimals = animalsInCategory.Count();
            ViewData["TotalPages"] = (int)Math.Ceiling((double)numberOfAnimals / PageSize);
            ViewData["CurrentPage"] = page;
            ViewData["CurrentCategory"] = category;

            return View(animalsInCategory
                            .Skip((page - 1) * PageSize)
                            .Take(PageSize)
                            .ToList()
                );
        }
    }
}

This approach will work, but what if you wanted to implement paging for another user interface? You would not be able to reuse the paging functionality in the controller and have to repeat yourself. This goes against the ‘Don’t Repeat Yourself (DRY) principle.

At this point some of you maybe be saying you ain’t gonna need it (YAGNI). Maybe… but let’s have a look at impact paging logic in the controller has when it comes to the unit testing the ASP.NET MVC controller.

using System.Collections.Generic;
using NUnit.Framework;
using Rhino.Mocks;
using TestingMvcControllers.Controllers;
using TestingMvcControllers.Interfaces;
using TestingMvcControllers.Models;

namespace TestingMvcControllers.Tests
{
    [TestFixture]
    public class AnimalsControllerTests
    {
        private IAnimalsRepository _animalsRepository;
        private AnimalsController _controller;

        [SetUp]
        public void SetUp()
        {
            _animalsRepository = MockRepository.GenerateStub<IAnimalsRepository>();
            _controller = new AnimalsController(_animalsRepository);
        }

        [Test]
        public void List_Presents_Correct_Page_Of_Animals()
        {
            // Arrange
            _controller.PageSize = 3;

            // Act
            _animalsRepository.Stub(a => a.Animals).Return(AnimalsStub.Animals);
            var result = _controller.List(null, 2);

            // Assert
            Assert.IsNotNull(result, "Didn't render view");
            var Animals = result.ViewData.Model as IList<Animal>;
            Assert.AreEqual(2, Animals.Count, "Got wrong number of animals");
            Assert.AreEqual(2, (int)result.ViewData["CurrentPage"], "Page number was wrong");
            Assert.AreEqual(2, (int)result.ViewData["TotalPages"], "Page count was wrong");
            Assert.AreEqual("Iguana", Animals[0].Name);
            Assert.AreEqual("Loin", Animals[1].Name);
        }

        [Test]
        public void List_Includes_All_Animals_When_Category_Is_Null()
        {
            // Arrange            
            _controller.PageSize = 10;            

            // Act
            _animalsRepository.Stub(a => a.Animals).Return(AnimalsStub.Animals);
            var result = _controller.List(null, 1);

            // Assert
            Assert.IsNotNull(result, "Didn't render view");
            var Animals = (IList<Animal>)result.ViewData.Model;
            Assert.AreEqual(5, Animals.Count, "Should have got 5 animals");
        }
        
        [Test]
        public void List_Filters_By_Category_When_Requested()
        {
            // Arrnage
            _controller.PageSize = 10;

            // Act
            _animalsRepository.Stub(a => a.Animals).Return(AnimalsStub.Animals);
            var result = _controller.List("Reptile", 1);

            // Assert
            Assert.IsNotNull(result, "Didn't render view");
            var Animals = (IList<Animal>)result.ViewData.Model;
            Assert.AreEqual(3, Animals.Count, "Should have got 3 animals");
            Assert.AreEqual("Crocodile", Animals[0].Name);
            Assert.AreEqual("Snake", Animals[1].Name);
            Assert.AreEqual("Iguana", Animals[2].Name);
            Assert.AreEqual("Reptile", result.ViewData["CurrentCategory"]);
        }
    }
}

Because the controller is also responsible for paging, you have to write unit tests for paging logic on top of the unit tests you should be writing for the controller. This means you will either ending up not testing enough and/or too much and find the tests a nightmare to maintain in the future. See my post about How to Unit Test ASP.NET MVC Controllers.

How I would refactor

I would move the paging functionality into a new layer. Yes I know adding another layer is always the solution to any problem in software development

The animal service layer looks like this

using System;
using System.Linq;
using TestingMvcControllers.Interfaces;
using TestingMvcControllers.Models;

namespace TestingMvcControllers.Services
{
    public class AnimalsService : IAnimalsService
    {
        private readonly IAnimalsRepository _animalsRepository;
        public int PageSize = 4;

        public AnimalsService(IAnimalsRepository animalsRepository)
        {
            _animalsRepository = animalsRepository;
        }

        public AnimalsDto GetAnimals(string category, int page)
        {
            AnimalsDto animalsDto = new AnimalsDto();

            var animalsInCategory =  (category == null)
                            ? _animalsRepository.Animals
                            : _animalsRepository.Animals.Where(x => x.Category == category);

            int numberOfAnimals = animalsInCategory.Count();
            animalsDto.CurrentCategory = category;
            animalsDto.CurrentPage = page;
            animalsDto.TotalPages = (int)Math.Ceiling((double)numberOfAnimals / PageSize);
            animalsDto.Animals = animalsInCategory
                .Skip((page - 1)*PageSize)
                .Take(PageSize)
                .ToList();

            return animalsDto;
        }
    }
}

Notice how a ‘Dto’ object is returned from the service. While this should be done using an AutoMapper, it’s not the focus of this test. The total number of pages can now be returned to the view as part of the model, rather than as a dictionary entry in ViewData.

The controller now looks clean and simple

using System.Web.Mvc;
using TestingMvcControllers.Interfaces;

namespace TestingMvcControllers.Controllers
{
    public class BetterAnimalsController : Controller
    {        
        private readonly IAnimalsService _animalsService;

        public BetterAnimalsController(IAnimalsService animalsService)
        {
            _animalsService = animalsService;
        }

        public ViewResult List(string category, int page)
        {
            var animalsDto = _animalsService.GetAnimals(category, page);
            return View("List",animalsDto);
        }
    }
}

The unit tests for the controller are about testing the controller and nothing more!

using MvcContrib.TestHelper;
using NUnit.Framework;
using Rhino.Mocks;
using TestingMvcControllers.Controllers;
using TestingMvcControllers.Interfaces;
using TestingMvcControllers.Models;

namespace TestingMvcControllers.Tests
{
    [TestFixture]
    public class BetterAnimalsControllerTests
    {
        private IAnimalsService _animalsService;
        private BetterAnimalsController _controller;

        [SetUp]
        public void SetUp()
        {
            _animalsService = MockRepository.GenerateStub<IAnimalsService>();
            _controller = new BetterAnimalsController(_animalsService);
        }

        [Test]
        public void The_List_Action_Returns_AnimalsDto_To_The_View()
        {
            // Arrange
            _animalsService.Stub(a => a.GetAnimals(null,0))
                .IgnoreArguments()
                .Return(new AnimalsDto());

            // Act
            var result = _controller.List(null, 0);

            // Assert            
            result.AssertViewRendered().WithViewData<AnimalsDto>();
        }

        [Test]
        public void The_List_Action_Returns_List_View()
        {
            // Arrange
            _animalsService.Stub(a => a.GetAnimals(null, 0))
                .IgnoreArguments()
                .Return(new AnimalsDto());

            // Act
            var result = _controller.List(null, 0);

            // Assert            
            result.AssertViewRendered().ForView("List");
        }
    }
}

Jag Reehal’s Final Thought on ‘Skinny ASP.NET MVC Controllers’

There is of course more than one way to skin a cat. This solution could have been refactored in many different ways.

The refactoring done in the post means:

• The ASP.NET MVC controller is skinnier with less responsibility which means it’s easier to read and maintain
• The controller unit tests can focus on the ‘subject under test’
• The paging functionality can be used by multiple user interfaces

Related Posts

1. How to Unit Test ASP.NET MVC Controllers