Refactoring a Controller Action - Caffeine-Powered Life

This is a response to Writing Maintainable Code by Michael Williamson. First off, I believe 100% in everything the author wrote. My only problem with his example is that he didn’t go nearly far enough down the refactoring rabbit hole.

Original Code

So let’s start with this original code, as presented by the author. I’ll warn you a couple of things.

It is quite awful.
It is typical of what I see when I look at other developers MVC code.


public class ArchiveController : Controller

{

  private readonly ISession m_Session;



  public ArchiveController(ISession session)

  {

    m_Session = session;

  }



  public ActionResult Index(FormCollection form)

  {

    var sds = form["start-date"];

    var sd = DateTime.MinValue;

    if (sds != null)

    {

        if (DateTime.TryParse(sds, out sd))

        {

        }

        else if (sds == "now")

        {

            sd = DateTime.Now;

        }

    }



    var eds = form["end-date"];

    var ed = DateTime.MaxValue;

    if (eds != null)

    {

        if (DateTime.TryParse(eds, out ed))

        {

        }

        else if (eds == "now")

        {

            ed = DateTime.Now;

        }

    }



    var hss = m_Session.QueryOver<HatSale>().Where(h => h.DateTime >= sd).Where(h => h.DateTime <= ed).List();



    var ahwpm = hss.GroupBy(h => h.DateTime.Month).Select(g => new {W = g.Key, Avg = g.Average(s => s.Hat.Width)});



    return View(new {Hss = hss, Ahwpm = ahwpm});

  }

}

Yes, the code compiles. Yes, the code works. But compiling and working isn’t the point (even though some developers think you’re done at this point). Compiling and working are just the first steps of a bigger process. We want code that is readable and maintainable over the long term. What are you going to see when you come back to it in 1, 3, 6, or 12 months (or longer)? No, the answer is not, “Let’s add a bunch of in-line comments.” The answer is refactor the working code into better code. Make smaller, reusuable, well-named methods.

Refactored Code - Part 1

The original author points out the following (all valid) problems with the original code.

The class is called ArchiveController – what’s in this archive?
The variables aren’t named well, for instance, what does ahwpm mean?
The logic that reads start-date and end-date from the form is duplicated.
What does DateTime on a hat sale represent?
There are bits of NHibernate querying and LINQ all on one unreadable line.
The method is long, and you feel you have to read the whole thing to begin to understand what’s going on.
The method operates at many layers of abstraction – it’s parsing strings, accessing the database, and doing some calculations.

The author then presents this updated solution.


public class SalesArchiveController : Controller

{

  private readonly ISession m_Session;

 

  public SalesArchiveController(ISession session)

  {

      m_Session = session;

  }



  public ActionResult Index(FormCollection form)

  {

    var startDate = ParseStartDate(form);

    var endDate = ParseEndDate(form);



    var hatSales = FetchHatSalesInDateRange(startDate, endDate);



    var averageHatWidthsPerMonth = CalculateAverageHatWidthsPerMonth(hatSales);



    return View(new {HatSales = hatSales, AverageHatWidthsPerMonth = averageHatWidthsPerMonth});

  }



  private DateTime ParseStartDate(FormCollection form)

  {

    return ParseDateParameterOrNull(form, "start-date") ?? DateTime.MinValue;

  }



  private DateTime ParseEndDate(FormCollection form)

  {

      return ParseDateParameterOrNull(form, "end-date") ?? DateTime.MaxValue;

  }

 

  private DateTime? ParseDateParameterOrNull(FormCollection form, string key)

  {

    var dateString = form[key];

    if (string.IsNullOrEmpty(dateString))

    {

        return null;

    }

    DateTime dateTime;

    if (DateTime.TryParse(dateString, out dateTime))

    {

        return dateTime;

    }

    if (dateString == "now")

    {

        return DateTime.Now;

    }

    return null;

  }

 

  private IList<HatSale> FetchHatSalesInDateRange(DateTime startDate, DateTime endDate)

  {

      return m_Session

          .QueryOver<HatSale>()

          .Where(h => h.DateOfSale >= startDate)

          .Where(h => h.DateOfSale <= endDate)

          .List();

  }

 

  private IList<MonthlyHatSales> CalculateAverageHatWidthsPerMonth(IList<HatSale> hatSales)

  {

      return hatSales

          .GroupBy(sale => sale.DateOfSale.Month)

          .Select(group => new MonthlyHatSales

          {

              AverageHatWidth = group.Average(sale => sale.Hat.Width),

              Month = group.Key

          })

          .ToList();

  }

 

  public class MonthlyHatSales

  {

      public int Month { get; set;}

      public double AverageHatWidth { get; set; }

  }

}

Is the refactoring better? Absolutely. Look at the action method now. Without a single inline comment, there is absolutely no doubt about what is going on, and there isn’t a single in-line comment. If you write clearly, you don’t need comments.

Maybe I should clarify that last sentence. You shouldn’t need comments for “what” your code is doing. “What” your code is doing will be self-discoverable when it is well written. I frequently find myself using “why” comments. “Why” am I solving the problem this way? Also, good comments in ugly code are better than no comments in ugly code. But the better solution is to not write ugly code in the first place. Anyway, here’s the refactored code.


var startDate = ParseStartDate(form);

var endDate = ParseEndDate(form);

var hatSales = FetchHatSalesInDateRange(startDate, endDate);

var averageHatWidthsPerMonth = CalculateAverageHatWidthsPerMonth(hatSales);

return View(new {HatSales = hatSales, AverageHatWidthsPerMonth = averageHatWidthsPerMonth});

My problems aren’t with what the author did; my problem is with how those aims were accomplished.

Why is MonthlyHatSales a nested class? What about this class makes it specific or dependent on the parent controller class?
Calculating the averages should be done as part of an extension method.
Why is the author parsing from the form collection?

Nested classes that have no dependency on their parent classes really bother me. More often than not, when I see a nested class, there’s no explanation beyond the original developer being to lazy to open up a new source file.

I think the last point is the biggest though. If you’re parsing the FormCollection, it shows me that you really don’t know the guts of MVC. You’re doing something that the architecture already does for you, and you’re not doing it as well as the architecture.

Refactored Code - Part 2

Have you seen FubuMVC? Even if you’re not using it, you should learn about it. The FubuMVC architecture has a wonderful pattern called one-model-in-one-model-out. I don’t use FubuMVC, but I love that idea. I copy it on my ASP.NET MVC projects. You have no idea how much this can clean up your code. First, let’s create an input model. We’re going to let the MVC model binder do the work for us. We’re also going to use proper properties (not auto-properties), since there is a bit of logic in the property getter.


public class SalesArchiveIndexInput

{

  private DateTime? startDate;

  private DateTime? endDate;

  

  public DateTime? StartDate 

  { 

    get { return startDate ?? DateTime.MinValue; }

    set { startDate = value; }

  }

  

  public DateTime? EndDate 

  { 

    get { return endDate ?? DateTime.MaxValue; }

    set { endDate = value; }    

  }

}

Now, we don’t need any parsing logic in our controller action at all, and we don’t need to worry about coercing a null value.

I’m going to leave the last two methods in there, but only because I don’t know the rest of the application. My bet is that the last method should be an extension method. They have no dependencies on the controller itself. They only depend on having an ISession instance and their input parameters.


public class SalesArchiveController : Controller

{

  private readonly ISession m_Session;

 

  public SalesArchiveController(ISession session)

  {

    m_Session = session;

  }



  public ActionResult Index(SalesArchiveIndexInput form)

  {

    var hatSales = FetchHatSalesInDateRange(form.StartDate.Value, form.EndDate.Value);

    var averageHatWidthsPerMonth = CalculateAverageHatWidthsPerMonth(hatSales);

    return View(new { HatSales = hatSales, AverageHatWidthsPerMonth = averageHatWidthsPerMonth });

  }



  private IList<HatSale> FetchHatSalesInDateRange(DateTime startDate, DateTime endDate)

  {

    return m_Session

        .QueryOver<HatSale>()

        .Where(h => h.DateOfSale >= startDate)

        .Where(h => h.DateOfSale <= endDate)

        .List();

  }

 

  private IList<MonthlyHatSales> CalculateAverageHatWidthsPerMonth(IList<HatSale> hatSales)

  {

    return hatSales

        .GroupBy(sale => sale.DateOfSale.Month)

        .Select(group => new MonthlyHatSales

        {

            AverageHatWidth = group.Average(sale => sale.Hat.Width),

            Month = group.Key

        })

        .ToList();

  } 

}

Now, there is absolutely no question about what is going on in that controller action, and we’re not duplicating anything that the framework already can do for us.

Original Code

Refactored Code - Part 1

Refactored Code - Part 2

Comments