Ayende @ Rahien

It's a girl

Refactoring toward frictionless & odorless code: Hiding global state

Originally posted at 3/30/2011

As I mentioned in the previous post, I don’t really like the code as written, so let us see what we can do to fix that. For a start, we have this code:

public class HomeController : Controller
{
    public ActionResult Blog(int id)
    {
        var blog = MvcApplication.CurrentSession.Get<Blog>(id);

        return Json(blog, JsonRequestBehavior.AllowGet);
    }
}

I don’t like the global reference in this manner, so let us see what we can do about it. The easiest way would be to just hide it very well:

public class HomeController : SessionController
{
    public ActionResult Blog(int id)
    {
        var blog = Session.Get<Blog>(id);

        return Json(blog, JsonRequestBehavior.AllowGet);
    }
}

public class SessionController : Controller
{
    public HttpSessionStateBase HttpSession
    {
        get { return base.Session;  }
    }

    public new ISession Session
    {
        get { return MvcApplication.CurrentSession; }
    }
}

So that result in nicer code, the architecture is pretty much the same, but we have a much nicer code for al of our controllers.

We are not done yet, though.

Comments

Ryan Heath
04/07/2011 09:29 AM by
Ryan Heath

I don't like renaming the existing property Session to HttpSession and newing another property for it.

I would rather do that the other way around to avoid any confusion for coders familiar with MVC.

Could you explain why you do prefer this over choosing a non conflicting property name?

// Ryan

Ayende Rahien
04/07/2011 09:33 AM by
Ayende Rahien

Ryan,

I very rarely using the HttpSession, and it is nicer to have the NH session available using just Session

Rob Kent
04/07/2011 10:00 AM by
Rob Kent

It's starting to smell like 'repository as a base class' pattern. You could add FindOne, Update, Delete, and Query methods to keep things simple for 90% of your requirements :)

Anton Gogolev
04/07/2011 10:02 AM by
Anton Gogolev

I prefer to expose a "ITransaction Transaction {get; set; }" property, which basically aggregates NHibernate's ISession and ITransaction. This way I can query the database, commit the underlying transaction right in the controller action and this does not conflict with an already-defined Controller.Session.

Avi Block
04/07/2011 10:02 AM by
Avi Block

Isn't that an abuse of inheritance?

Scooletz
04/07/2011 10:06 AM by
Scooletz

LayerSuperType is unneeded here and can be removed easly. I hope some ModelBinders will be shown to inject session right as an action parameter:)

Rob Kent
04/07/2011 10:15 AM by
Rob Kent

This design makes unit testing difficult doesn't it? MvcApplication is now bound up with your controller.

I still prefer dependency injection of ISession or, heaven forfend, IRepository into the constructor.

Owen
04/07/2011 10:18 AM by
Owen

Is this a late April Foils joke?

Seriously though, I don't see what you have gained from this. You still have a global reference, even if it is hidden away in a base class.

I would have expected something better smelling, like using IoC to provide the session to the controller.

tobi
04/07/2011 10:28 AM by
tobi

Owen, now that the Session is behind a property he can easily make the session come from wherever he wants. This was surely only the first step. I use this pattern too. And yes, it is an abuse of inheritance which is well worth it.

Nick Aceves
04/07/2011 10:35 AM by
Nick Aceves

@Owen,

Did you even read the last two lines of the post?

Linkgoron
04/07/2011 11:11 AM by
Linkgoron

@Scooletz Modelbinders for a session inside the Action?! that's insane. I would guess (hope?) that the next step would be constructor injection, although it seems we're moving towards setter injection.

Ayende I think that you should've made this a longer post. This still smells and all you've done is hide the problems that existed before (and added some new ones). This only seems better (from the derived class) but actually isn't.

AndersM
04/07/2011 11:46 AM by
AndersM

Why can you not understand that this is only a little step in resolving the larger issue? Actually try to read his post, and not just look at the code. Context matters!

DaveW
04/07/2011 11:46 AM by
DaveW

@Linkgoron and @Owen: seeing that the next post is subtitled "Limiting Session Scope", I wouldn't panic/pass judgement just yet. Showing how you can take incremental steps to a better design is better than just saying, don't do that, do this instead.

Especially when I send the links to one of my devs in a few months time...

Rob Kent
04/07/2011 12:11 PM by
Rob Kent

He's teasing us.

Linkgoron
04/07/2011 12:43 PM by
Linkgoron

@dave my main criticism is that this post should've been longer, with the next step included. I think the message would've been clearer if the post included more content.

This refactoring is supposed to last a week, so I guess that's why we've got these small steps. However, I do really hope that the Session property hiding is a joke.

pete w
04/07/2011 02:18 PM by
pete w

syntactic sugar

naraga
04/07/2011 03:44 PM by
naraga

small step for this example snippet but big step for overall architecture. V2 is not only nicer but it hides away session retrieval concern to one (and only one) place.

let's see what V3 will bring :)

Alessandro Riolo
04/07/2011 04:30 PM by
Alessandro Riolo

I actually like the approach of this series, even at this early stage I feel the most interesting aspect is going to be the refactoring approach.

Nick
04/07/2011 06:21 PM by
Nick

I'm gonna hack ayende's database and find out what's in the next blog post. Then I'm gonna come back and look all smart by you telling you what he's going to do next.

See you in 5.

Russell
04/07/2011 11:53 PM by
Russell

Is this a late April fools joke? I really hope so!

It has to be the dumbest idea ever to replace functionality which everyone knows and uses. Just create a NSession property or Nession or whatever, but don't replace the Session property.

Dmitry
04/08/2011 12:18 AM by
Dmitry

Why not just use dependency injection?

jan
04/08/2011 06:13 AM by
jan

@Dmitry

As already stated before, he is not done yet...

I'm a bit curious, too, how it's going on...

Jon Rowett
04/08/2011 08:43 AM by
Jon Rowett

inject the session, surely?

Comments have been closed on this topic.