Separation of Concerns – how not to do it

In a recent article on layered LINQ applications in the latest ASP.NET PRO magazine (no link, you have to pay), the author proposes separating your application into three distinct layers:

• User Interface (UI) layer
• Business Logic Layer (BLL)
• Data Access Layer (DAL)

I certainly would have agreed, at least back in 2004 or so.  Many of the sample applications coming out of MSDN have a similar architecture, with namespaces like Northwind.DAL to reinforce that architecture.

While separating out concerns into these layers is a good first step, it’s only the first step.  Layered architecture and separation of concerns goes well beyond splitting your code into three classes.  Our code itself gives hints for when a class is concerned with too much going on.

###

Too many concerns

A snippet of the BLL class looked something like this:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Web;

public class CustomerManager
{
    [DataObjectMethod(DataObjectMethodType.Select, false)]
    public static List<Customer> GetCustomers(int startRowIndex, int maximumRows)
    {
        List<Customer> customers = null;
        string key = "Customers_Customers_" + startRowIndex + maximumRows;

        if (HttpContext.Current.Cache[key] != null)
        {
            customers = (List<Customer>) HttpContext.Current.Cache[key];
        }
        else
        {
            customers =
                (
                    from
                        c in DataGateway.Context.Customers
                    orderby
                        c.CustomerID descending
                    select
                        c
                ).Skip(startRowIndex).Take(maximumRows).ToList();

            if ((customers != null) && (customers.Count > 0))
                HttpContext.Current.Cache.Insert(key, customers, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
        }

        return customers;
    }
    
}

This CustomerManager class would also have methods for all of the CRUD operations, delegating to LINQ to SQL for the heavy lifting.  Already we have some inconsistencies and pain points with this class:

• It manages CRUD operations, yet is situated in the BLL.  Is it data access or not?
• The name is nebulous.  It Manages Customers, but how?  What aspects?  Persistence?  Caching?  Business Rules?  It might as well implement IKitchenSink.
• Persistence and caching concerns are mixed together
• Customer management and UI concerns are mixed together (i.e., the DataObjectMethod attribute)
• Not an intention-revealing interface – List allows me to add, remove and insert items to the list.  This should be a read-only affair.

Outside of these issues, there are other problems we can see off-hand:

• Potential for InvalidCastException – anyone can stick something in Cache with that key.  Use the “as” operator instead
• The “customers” variable is never null.  No need to check it.
• Several static dependencies – the DataGateway.Context and HttpContext.Current static properties
• The whole CustomerManager class is impossible to unit test

Or test in any way possible.  For me to add an automated test, the whole HttpContext needs to be up and running.  I’ll get an immediate NullReferenceException as soon as any piece of code hits HttpContext.Current.

Layering applications is a great step in the right direction to removing concerns from the ASP.NET codebehind.  But it’s only the first step.  Next time, we’ll see how we can split out the many concerns from this CustomerManager.