Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 5,972 | Comments: 44,523

filter by tags archive

Refactoring toward frictionless & odorless codeHiding 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.

More posts in "Refactoring toward frictionless & odorless code" series:

  1. (12 Apr 2011) What about transactions?
  2. (11 Apr 2011) Getting rid of globals
  3. (10 Apr 2011) The case for the view model
  4. (09 Apr 2011) A broken home (controller)
  5. (08 Apr 2011) Limiting session scope
  6. (07 Apr 2011) Hiding global state
  7. (06 Apr 2011) The baseline

Comments

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

Ryan,

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

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

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

Isn't that an abuse of inheritance?

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

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

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

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

@Owen,

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

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

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

@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

He's teasing us.

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

syntactic sugar

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

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

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

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

Why not just use dependency injection?

jan
jan

@Dmitry

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

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

Jon Rowett

inject the session, surely?

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. Production postmortem (5):
    29 Jul 2015 - The evil licensing code
  2. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
  3. API Design (7):
    20 Jul 2015 - We’ll let the users sort it out
  4. What is new in RavenDB 3.5 (3):
    15 Jul 2015 - Exploring data in the dark
  5. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats