views:

184

answers:

4

In the tutorials for ASP.Net MVC, the LINQ to Entities code looks like:

public class MyController : Controller
{
    private Models db;

    public ActionResult Index()
    {
        db = new Models();
        var foo = (from f in db.foo select f).ToList();
        return View(foo);
    }
}

I'm guessing this has something to do with thread safety/connection pooling, but I just wanted to know if anyone knew of any good reasons not to do it this way:

public class MyController : Controller
{
    private readonly Models db = new Models();

    public ActionResult Index()
    {
        var foo = (from f in db.foo select f).ToList();
        return View(foo);
    }
}
A: 

the first code snippet shouldn't compile. you are assigning a readonly field in a method that is not a constructor. are you sure it's an MVC sample?

Ali Shafai
Copy pasta, fixed now.
tghw
cool, I just don't understand why someone didn't like me because I found a typo :)Cheers,Ali
Ali Shafai
+1  A: 

From the code samples I've seen you should write the code your looking for as follows:

public class MyController : Controller{
    public ActionResult Index()
    {
        using (var db = new Models())
        {
            var foo = db.Foo.ToList();
            return View(foo);
        }
    }
}

There are two main changes here.

  1. The EF Model is an IDisposable object and should be disposed of properly. It's not going to destroy your app if you don't (GC will clean it up later), but it's best to clean it up early if you can.
  2. There is really no need for the from/select stuff in your LINQ, it's not doing anything in this scenario.

Although you should consider your application architecture as a whole and think about abstracting your data access outside of your controller.

bendewey
+1  A: 

Keeping it inside the method ensures that the db will only be instantiated when its needed and added to the connection pool when needed as well. I would even take the next step and instantiate it inside a using statement.

public class MyController : Controller
{
    public ActionResult Index()
    {
        using (Models db = new Models())
        {
            var foo = (from f in db.foo select f).ToList();
            return View(foo);
        }
    }
}
David Yancey
+4  A: 

I just put together a tip that covers this in quite a lot of detail.

Your educated guess of threading is just one of the many reasons why it is generally better to construct/dispose of the Context in the method that needs it.

There are some situations where this rule of thumb doesn't hold but they are pretty rare.

See this: Tip 18 - How to decide on a lifetime for your ObjectContext

Alex James
Thanks, that's what I was looking for. Good article!
tghw