Put your controllers on a diet: defactoring

Posts in this series:

Before getting to far in refactoring my controller, I want to spend a little bit of time doing some defactoring, removing abstractions and such until I can see the entire amount of non-framework code in one spot. Defactoring is often an important first step in refactoring; you really need a big picture view of your code so that you don’t get sucked in to abstraction-itis.

First up is getting rid of data access abstractions. Since I’m using mostly NHibernate directly and not abstractions there, it shouldn’t be too bad. I have three main interfaces, first representing a transactional boundary:

public interface ITransactionBoundary : IDisposable
{
    ISession CurrentSession { get; }
    void Begin();
    void Commit();
    void RollBack();
}

Not horrible, but the implementation is…strange. But there’s not really a big reason to have this abstraction. Transaction management should be taken care of through infrastructure, and not in your application layer. Transactions should be ambient, not imperatively managed in my controller. On top of that, NHibernate already includes transaction management and unit of work representation, through the ISession interface. Basically all we have above is an ISession wrapper with wonky internal semantics (I wrote the code, so I’m really talking to myself).

Next, we have ISessionSource:

public interface ISessionSource
{
  ISession CreateSession();
  void BuildSchema();
}

To be perfectly honest, I don’t remember what I was thinking here. Dependency injection should take care of getting an ISession supplied to us. In the above interface, I don’t know if that method creates a new ISession or re-uses an existing one. Altogether, not a good spot to be in when you have to look at an implementation of an interface to understand behavior. Strike two.

Finally, we have our repository interface:

public interface IRepository<T> where T : Entity
{
  T GetById(Guid id);
  void Save(T entity);
  IEnumerable<T> GetAll();
  INHibernateQueryable<T> Query();
  void Delete(T entity);
}

Again, not much value to this interface as an encapsulation. I still expose NHibernate out through the Query method (absolutely required if I want to do fetching etc.), and for some cases, I can’t even use this abstraction. NHibernate already provides an abstraction through ISession, so let’s just get rid of all these abstractions.

Ideally, I’d just depend directly in ISession in my controller and use it without any abstractions before we go down the road of refactoring. Removing these layers is like excavating down to the bedrock before laying foundation for a building. I need to strip away the muck and mud to get at the base. I can modify my StructureMap registration code to allow me to depend directly on ISession and encapsulate lifecycle management inside the container:

public class NHibernateRegistry : Registry
{
  public NHibernateRegistry()
  {
    For<Configuration>().Singleton().Use(c => new ConfigurationFactory().AssembleConfiguration());
    For<ISessionFactory>().Singleton().Use(c => c.GetInstance<Configuration>().BuildSessionFactory());
    For<ISession>().HybridHttpOrThreadLocalScoped().Use(c =>
    {
      var sessionFactory = c.GetInstance<ISessionFactory>();
      return sessionFactory.OpenSession();
    });
  }
}

Now we don’t have to worry about error-prone lifecycle management code, locks and such. All that code can go away. Aren’t containers nice?

Let’s go back to our controller to see how this changed it:

public class ConferenceController : Controller
{
    private readonly ISession _session;

    public ConferenceController(ISession session)
    {
        _session = session;
    }

    public ActionResult Index(int? minSessions)
    {
        minSessions = minSessions ?? 0;

        var list = (from conf in _session.Query<Conference>()
            where conf.SessionCount >= minSessions
            select new ConferenceListModel
            {
                Id = conf.Id,
                Name = conf.Name,
                SessionCount = conf.SessionCount,
                AttendeeCount = conf.AttendeeCount
            }).ToArray();

        return View(list);
    }

    public ViewResult Show(string eventName)
    {
        var conf = _session
            .Query<Conference>()
            .SingleOrDefault(c => c.Name == eventName);

        var model = new ConferenceShowModel
        {
            Name = conf.Name,
            Sessions = conf.GetSessions()
                .Select(s => new ConferenceShowModel.SessionModel
                {
                    Title = s.Title,
                    SpeakerFirstName = s.Speaker.FirstName,
                    SpeakerLastName = s.Speaker.LastName,
                }).ToArray()
        };

        return View(model);
    }

    public ActionResult Edit(string eventName)
    {
        var conf = _session
            .Query<Conference>()
            .SingleOrDefault(c => c.Name == eventName);

        var model = new ConferenceEditModel
        {
            Id = conf.Id,
            Name = conf.Name,
            Attendees = conf.GetAttendees()
                .Select(a => new ConferenceEditModel.AttendeeEditModel
                {
                    Id = a.Id,
                    FirstName = a.FirstName,
                    LastName = a.LastName,
                    Email = a.Email,
                }).ToArray()
        };

        return View(model);
    }


    [HttpPost]
    public ActionResult Edit(ConferenceEditModel form)
    {
        if (!ModelState.IsValid)
        {
            return View(form);
        }

        var conf = _session.Get<Conference>(form.Id);

        conf.ChangeName(form.Name);

        foreach (var attendeeEditModel in form.Attendees)
        {
            var attendee = conf.GetAttendee(attendeeEditModel.Id);

            attendee.ChangeName(attendeeEditModel.FirstName, attendeeEditModel.LastName);
            attendee.Email = attendeeEditModel.Email;
        }

        return this.RedirectToAction(c => c.Index(null), "Default");
    }
}

From here, we have a much better target to start refactoring.

At this point, however, you might ask yourself “is there anything to refactor here”? Looking at this isolated controller, no. All behavior is neatly encapsulated inside this one class. However, if I have a dozen or two dozen controllers that look awfully similar, and we start to see cross-cutting concerns pop up, then we’ll look at refactoring (but not before).

In the next installment, we’ll look first at those immediately obvious cross cutting concerns, validation and transaction management.

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 ASP.NET MVC. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • darrencauthon

    I wish the next installment was on how to unit test controllers written this way.

    • Mike Cole

      +1

    • jbogard

      What is the value of unit testing controllers written this way?

      • darrencauthon

        The most obvious answer is that we have to verify that this controller works as expected and required. A properly-functioning application hinges on a lot of small details thrown into this controller — things like the ToArray, the building of the models, the SingleOrDefault, the IsValid check, saving values from the post, looping through the attendees, etc. If any of these bits of code are changed, the application may not work. We shouldn’t be afraid to edit code like this.

        And gosh, do we even know if it works? In your HttpPost Edit action, you loop through the attendees. You call ChangeName on the model (??????), then you set “attendee.Email = attendeeEditModel.Email”. Does your *property* make the necessary calls to update the attendee’s email address? Is the save made in the garbage collector? Just looking at the code (which is all I can do without tests), this looks like a possible bug. Drop this code in Github, and let’s run it! :)

        I know your post is on “putting controllers on a diet” and you’re changing around the code for some purpose. I’ve done that a lot, too, but the first step is always to put some sort of tests around the code to verify that it continues to work as I tinker. They don’t even have to be unit tests — they just need to be something to verify these details. Perhaps the controller “unit” test will morph into an integration test as the refactor or defactor or tinkering is done to it.

        But other than verifying the code works, I think some tests would provide some better design. Like I said, this controller breaks SRP in a number of different ways, but it’s still relatively simple. To me, that simplicity signals that it’s probably better to delete it all and start fresh – *not* that we don’t have to test it at all.

        • jbogard

          Long answer to a question asking if we should *unit* test. Not just *test*. I agree this should be tested, but unit testing would require encapsulating/abstracting of components, to which there is a cost. That needs to be balanced in the complexity of the app. When I defactored, I didn’t change behavior, I just pulled all the code back up to the front.

          • Mike Cole

            So what kinds of tests are you suggesting? Controller tests, selenium-type tests, human testing? It’s refreshing to hear your approach and I’m genuinely interested to see how much testing to feel is enough in this case. Thanks!

          • jbogard

            Integration tests at the controller level, just instantiating a controller with a live ISession, calling the action method with real data, and verifying the action result on the other side.

          • Mike Cole

            Thanks.

          • darrencauthon

            “I agree this should be tested,”

            That’s why I’d like to see the tests. I think there’s a lot of assumptions and pre-built biases in declaring code like this. You’d say you wanted to test it, but really what you must means is that you want to test “some” of it. The broad details would be covered with some very broad tests, but the fine details (each of which could break the app) would just be glossed over.

            Unit testing requires encapsulating and abstracting components, but not in the same way that I think they’re commonly done in C#. The split isn’t on abstracting the database or core components, it’s on abstracting the needs of the application. Like this app — you need to show and edit conferences. Let *that* be the abstraction, let TDD force the need of it, and then work your way down.

            ” When I defactored, I didn’t change behavior”

            Refactoring/Defactoring is the changing of the code without changing its behavior – so how do you know you didn’t change the behavior without some way to verify?

            I don’t mean to derail you on this subject, but I feel that testing is incredibly important for *any* defactor/refactor of a codebase – but it’s rarely discussed in this context.

          • jbogard

            Hmm, I guess I didn’t discuss the tests because I thought they’d be simple and obvious. Here’s my test https://gist.github.com/jbogard/7136623 which really wouldn’t have changed with refactoring/defactoring. Completely divorced from implementation.

          • darrencauthon

            Does this test run and/or pass? If so…

            How does the attendee email get set? You set the email address using the Email setter, and then somehow, automagically, it’s going to make the change in data storage? Either you’d have logic in the setter, or the test can’t pass.

          • jbogard

            Yeah the test passes. What automagic logic are you talking about? I make a change, commit the transaction, and it gets persisted in the DB. Details of transaction scope are encapsulated in my fixture (as it already is in my app). Is it because you’re seeing that I don’t explicitly call “Save” or “update” or something like that on the session? These entities are POCOs, but NH is doing all the dirty checking and commits/flushes with the transaction. I emulate this in my test by doing fixture.CreateInstance, then fixture.CommitChanges(), as my app does with the controller factory and creating the transaction scope around my action (not inside).

          • darrencauthon

            I’m blown away that that sort of scoping happens… I guess I’m expecting some sort of verb to express the intent that something changes. I’ve always considered objects in memory, including their properties, as “safe” unless some action is taken. You declare your intent to change the name with “ChangeName”, but the email is just done with a setter.

            Maybe another way to look at this… I rely on dirty checking, too, but we don’t dirty check whatever’s in memory — we dirty check those objects that the programmer told to update. Operations are declared, and never left to chance or implied intent.

            I’ve been doing full-time Ruby for a few years after 7+ with .Net… and I thought the magic in Ruby was bad! :)

            I’ll drop it now, though… have a good day!

          • jbogard

            Yes, this is what modern ORMs let you do. Automatic dirty checks, so that developers don’t forget to call “Save” on something. Just one less thing to forget and screw up. RavenDB works this way too, you insert documents, but only call Update if you want to update *that current snapshot of the doc*, yet another thing that could get screwed up if the object is mutated after you call Save.

            With NH, we work with a Unit of Work that captures all the work we have done for us without us doing extra work for it.

          • darrencauthon

            BTW, what about that Email property setting? I’d still like to know the effect of that code; I just can’t see it.

  • Mike Cole

    Where’s the code you’re referencing?

  • Daniel Marbach

    This works nice as long as your only querying stuff. But you need Sqllite, Compact or anything alike for all those controllers. As soon as you need to make educated guesses in top of the data your are querying this approach has its drawbacks because infrastructure is leakimg into each controller. This way your also building data centric and not domain centric applications. It is all about tradeoffs

    • jbogard

      I wasn’t really suggesting writing apps this way – more that in order to refactor, we often need to defactor first. With all those abstractions, we don’t have a great direction to go in.

      • Mike Cole

        I look forward to future posts in this series. Very applicable to me at this point.

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1469

  • Jon

    I have to ask…how did you get your Gists embedded with a dark theme?

    • jbogard

      lol I have no idea, something in our WordPress I guess? I didn’t set it up, it’s just a WordPress plugin

      • Jon

        Dang, I was hoping to copy it :p

  • Paul Connolly

    Be careful with NHibernate and using a container like Autofac, there is a memory leak with NHibernate which will not clear down an ISession correctly if there is a delegate holding on to it. We had to set the ISession to ExternallyOwned and handle the teardown ourselves (through infrastructure also)

    • Mike Cole

      Is this documented somewhere?

      • Paul Connolly

        No, but objects in the first level cache will be held onto if even a skeleton disposed ISession is lingering around.

  • James Nail

    Jimmy, I’ll say one thing that the abstraction over ISession has going for it. NHibernate’s LINQ support still comes in the form of an extension method, so it becomes pretty tricky to mock/stub that guy.

    • jbogard

      In practice, I don’t use the LINQ support much, LINQ is just too limiting. I’d rather have something built for that ORM.

      • James Nail

        Agreed, LINQ is pretty much the lowest common denominator there. Encapsulating the various implementation-specific parts is a big factor that pushes me toward (non-generic) repositories. I know you’re a big proponent of query objects, so that’s another good way too.

  • mxmissile

    Jimmy where are you getting INHibernateQueryable? That interface is new to me.

    • jbogard

      I think it’s an old interface from NHibernate.Linq.

  • Pingback: Put your controllers on a diet: POSTs and commands | Jimmy Bogard's Blog