+972 52-548-6969
Refactoring toward frictionless & odorless codeHiding global state
time to read 2 min | 397 words
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:
- (12 Apr 2011) What about transactions?
- (11 Apr 2011) Getting rid of globals
- (10 Apr 2011) The case for the view model
- (09 Apr 2011) A broken home (controller)
- (08 Apr 2011) Limiting session scope
- (07 Apr 2011) Hiding global state
- (06 Apr 2011) The baseline
Comments
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
Ryan,
I very rarely using the HttpSession, and it is nicer to have the NH session available using just Session
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 :)
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.
Isn't that an abuse of inheritance?
LayerSuperType is unneeded here and can be removed easly. I hope some ModelBinders will be shown to inject session right as an action parameter:)
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.
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.
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.
@Owen,
Did you even read the last two lines of the post?
@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.
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!
@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...
He's teasing us.
@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.
@Linkogoron,
I've done it and it's the most atomic thing you can do, if you have controller with a parameter of its constructor used in only one action. Take a look mate: blog.scooletz.com/.../asp-mvc-model-binders/
syntactic sugar
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 :)
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.
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.
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.
Why not just use dependency injection?
@Dmitry
As already stated before, he is not done yet...
I'm a bit curious, too, how it's going on...
inject the session, surely?
Comment preview