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<Customer> 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.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in LINQ to SQL, Refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.davetheninja.net Dave The Ninja

    That is really frightening!

    These articles are supposedly targeted at people who are learning/brushing up or their skills.

    Were supposed to be sharing information/practices/patterns through out the community, Alt.net, and articles like this truly have the potential to put a spanner in the works, inadvertantly teaching people the wrong way to do something – causing the rest of us extra work making things right again.

    Very concerning

    Dave The Ninja

  • http://www.davetheninja.net Dave The Ninja

    …Oh, and ASP.net Pro mag have the cheek to charge for these conter productive articles!…..

    rant over, sorry for polluting your blog Jimmy

    Dave The Ninja

  • http://www.blogcoward.com jdn

    Then how about you submit an article to Asp.Net PRO showing better practices?

    Some of the other ‘thought leaders’ are stepping up to the plate and contributing to MSDN Mag, et. al. instead of just blogging one-offs.

    The ‘at least back in 2004 or so’ line is funny though.

  • http://Bryan.ReynoldsLive.com Bryan Reynolds

    This has gone on for years.

  • rkg

    I love it! It’s rare to see an entry that breaks down a design and discusses it’s ramifications. I understand that samples in books and articles are usually over simplified to allow people to understand and follow them in a short amount of time. However, as a jr. developer who is trying to upgrade his skills, I rarely find adequate material that presents methods and principles that you would actually use in a production application. I’m sure that the author of the ASP.NET Pro mag knows better but people like me don’t and so those articles are often more harm than good. I can’t wait for your next post and see how you implement the solutions to the problems you pointed out!

  • http://colinjack.blogspot.com Colin Jack

    Awful, but then I trust very little in any of these magazines…over-simplified nonsense in general. Even when they do make an effort, as in when they do put an emphasis on design/testing, they miss the subtleties and so render the content meaningless.

    Thing is, the people who read that sort of article and think it’s showing good design are probably not going to be out actively looking for ways to improve their designs.

  • http://flimflan.com/blog Joshua Flanagan

    I agree with the overall design is flawed, but I think I prefer the unsafe cast as opposed to “as” in this scenario. When you say “anyone” can stick something in the cache, I assume you mean other developers. If other developers on your team are using the same cache key for different purposes, I think I’d want to know as soon as possible (fail fast). If an exception isn’t thrown, you will never know that you have a bug, the code will just silently continue to get a cache miss even when the data should be in cache.

    I would, however, change the cache key creation so that I don’t get the cached results for 1, 15 when I ask for 11, 5.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Dave

    So much pollution!!

    It’s still a huge improvement over everything in the code-behind though.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @jdn

    Have you _seen_ the cover graphic that article had? I don’t have that kind of talent!

    That article was already written, many times over, over the past couple of years. Check out the recent articles in the MSDN magazine.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @rkg

    “Bad” is relative – it’s better than other styles I’ve seen. It could all be jammed in a code-behind.

  • http://www.lostechies.com/blogs/sean_chambers Sean Chambers

    My god, what is that awful smell???

    oh wait, that’s just the snippet of code from a microsoft magazine…whoops!

    The worst part of the whole deal is someone (probably in the thousands), will read that article and assume that since it is from a MS mag, it must be considered best practices and go make their logic look the same and act the same.

    that is the true horror at hand!

  • SOC Noob

    To make something positive out of the article, any of the more experienced here care to post some code on how to refactor this example to have better separation of concerns?

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Noob

    I’m planning on a follow-up post. I’m contemplating on a screencast, as the decisions on direction are more important than the final destination.

  • http://brackett@ufl.edu Mark Brackett

    In the mag’s defense, I’d guess that there’s hardly any business logic in the whole app. Most apps (espcially MS samples) are thin wrappers over SQL Server data.

    In that case, having your “Business Logic Layer” be, essentially, view formatted and accessible data, is pretty much the norm.

    I’m not fond of the HttpContext.Current call, or the direct management of Cache – but neither are huge sins.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Mark

    Nope, definitely not huge sins given the scope of this app. But it does violate SoC. It’s a choice whether or not you want to adhere to that principle.

    Testability, however, is always a concern. That governs the long-term maintainability of the system. If this app didn’t have any business logic, why even build a three-tier application?

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Josh

    Re: cache key problem

    That was a problem in my code, not in the original article. Whoops.