Returning IQueryable from within a using block. Need better design

Question

I have created a PhoneBook style application; on my phonebook object I have a local member _site which is used as a filter, since there are approximately 1000 phone numbers, split across 12 sites within my organisation. Only one site will be retrieved at a time using this method.

This was my original method. The GUI has several methods for reordering the data, so I left it as an IQueryable because I would like to defer SQL to allow for filtering to be done on the SQL server rather than on the client PC.

Works

public IQueryable<PhoneNumber> GetPhoneDirectory()
{
    PhoneBookDataContext db = new PhoneBookDataContext())
    return db.PhoneNumbers.Where(d => d.Site == _site);
}

However, I am also trying to keep to 'best practise' in terms of using statements.

Doesn't Work

public IQueryable<PhoneNumber> GetPhoneDirectory()
{
    using (PhoneBookDataContext db = new PhoneBookDataContext())
    {
        return db.PhoneNumbers.Where(d => d.Site == _site);
    }
}

Now as pointed out by @justanotheruseryoumay, this will cause an exception because the datacontext is disposed by the time the objects are accessed.

I guess what I am asking is, how can I make sure my data context is disposed nicely, when I cannot use a 'using' statement and don't strictly know when the context is done with.

Answer 1

If you want to return IQueryable you can make your class that contains the GetPhoneDirectory disposable, make the PhoneBookDataContext a field, and dispose it in your dispose method.

You will then put the onus on the caller to dispose his instance of your class.

E.g.

public class MyClass : IDisposable
{
    PhoneBookDataContext db;

    public MyClass()
    {
        db = new PhoneBookDataContext();
    }

    public IQueryable<PhoneNumber> GetPhoneDirectory()
    {
        return db.PhoneNumbers.Where(d => d.Site == _site);
    }

    public void Dispose()
    {
        if (db != null)
        {
            db.Dispose();
            db = null;
        }
    }
}

// Caller
using(var myClass = new MyClass())
{
    var queryable = myClass.GetPhoneDirectory();
    ...
}

Answer 2

The execution of the query will still be deferred and the PhoneBookDataContext will still be properly Disposed because using is interpreted by the compile as a try/finally. When you actually execute the query it will result in a runtime error because the PhoneBookDataContext no longer exists. I would suggest doing a .ToList() on your query and returning it that way. If you want to change the order after you return it then you can still do LINQ on it as you please.

EDIT: Another thing you could do is to create the using with the PhoneBookDataContext in the calling method and pass in the context. The context is really going to be used in that method anyway and you can keep it around as long as you need it and stick with the good using format.

Answer 3

Yes; It is bad design because your IQueryable<PhoneNumber> will be evaluated only when you call a method that cause it to be evaluated, like ToList() or when you iterate it with foreach.

in your code you are returning a IQueryable<PhoneNumber> which is not evaluated yet, and before the caller get any chance to execute it, it's internals that has the responsibility for yielding the records to you (db); is already disposed.

Just as a suggestion:

public IEnumerable<PhoneNumber> GetPhoneDirectory()
{
    using (PhoneBookDataContext db = new PhoneBookDataContext())
    {
        return db.PhoneNumbers.Where(d => d.Site == _site).ToList();
    }
}

Or relocate the db object to somewhere else in your design (Unit Of Work and Repository are nice patterns to get a look at IMHO).