Should I implement IDisposable here?

Question

My method which calls SQL Server returns a DataReader but because of what I need to do - which is return the DataReader to the calling method which resides in a page code-behind - I can't close the connection in the class of the method which calls SQL server. Due to this, I have no finally or using blocks.

Is the correct way of disposing resources to make the class implement IDisposable? Alternatively should I explicitly dispose the unmanaged resource (class-level fields) from the caller?

EDIT: I send the datareader back because I need to bind specific data from the datareader to a listitem control, so in the calling class (Codebehind page), I do:

 new ListItem(datareader["dc"]); (along those lines).

Answer 1

I would say yes, implement IDisposable. One of the main reasons as far as I can tell to use it is when you cannot trust the user of the object enough to do it properly themselves. This seems to be a prime candidate for that.

This being said however, there is a question to your architecture. Why is it that you want to send the DataReader itself to the page instead of calling a method to do it for you (including the relevant cleanup) by returning what is necessary? If it necessary to give the actual reader to the page, then so be it.

Answer 2

Yes you should implement IDisposable on your custom class if it contains a DataReader that is open when returned to a lower layer.

It is the accepted pattern when returning something, that needs to be cleaned up.

Answer 3

Your class

class MyClass : IDisposable
{
  protected List<DataReader> _readers = new List<DataReader>();
  public DataReader MyFunc()
  {
      ///... code to do stuff

      _readers.Add(myReader);
      return myReader;
  }
  private void Dispose()
  {
      for (int i = _readers.Count - 1; i >= 0; i--)
      {
          DataReader dr = _reader.Remove(i);
          dr.Dispose();
      }
      _readers = null;

      // Dispose / Close Connection
  }
}

Then outside your class

public void FunctionThatUsesMyClass()
{
   using(MyClass c = new MyClass())
   {
       DataReader dr = c.MyFunc();
   }
}

All the readers and the MyClass instance get cleaned up when the using block exits.

Answer 4

First, passing the DataReader may not really be what you want to do, but I'll assume that it is.

The proper way to handle this would be to pass back a composite type that either encapsulates or exposes the DataReader and holds on to the connection, then implement IDisposable on that type. When disposing of that type, dispose both the reader and the connection.

public class YourClass : IDisposable
{
    private IDbConnection connection;
    private IDataReader reader;

    public IDataReader Reader { get { return reader; } }

    public YourClass(IDbConnection connection, IDataReader reader)
    {
        this.connection = connection;
        this.reader = reader;
    }

    public void Dispose()
    {
        reader.Dispose();
        connection.Dispose();
    }
}

Answer 5

Holding the database connection as a member variable in your reader class and making your reader class implement IDisposable seems fine to me.

However, you might consider making your method return IEnumerable and using yield return statements to walk through the data reader. That way you can return the results and still clean up from within your method.

Here's a rough sketch of what I mean:

public IEnumerable<Person> ReadPeople(string name)
{
    using (var reader = OpenReader(...))
    {
        // loop through the reader and create Person objects
        for ...
        {
            var person = new Person();
            ...
            yield return person;
        }
    }
}

Answer 6

The general rule is that your class should implement IDisposable if it directly holds unmanaged resources or holds a reference to another IDisposable object. If your class creates an IDataReader in one method but never holds that reference then your class would not need to implement IDisposable per the rule (unless it just happens to hold an IDisposable aside from the IDataReader created in that one method).

The real question you need to ask yourself is whether your class really should be holding onto that IDataReader even after it has delivered it to the caller. Personally, I think that is a poor design because it blurs the line of ownership. Who actually owns the IDisposable in that case? Who is responsible for its lifetime? Take the IDbCommand classes for example. They create IDataReader instances and return them to the callers, but absolve themselves of ownership. That makes the API clean and the responsibility for lifetime management is unambiguous in that case.

Regardless of the ownership issue your specific situation necessitates implementing IDisposable; not because your class happens to create and return an IDataReader instance, but because it sounds like it holds an IDbConnection object.

Answer 7

I wouldn't return anything. Instead, I would pass a delegate.

For example:

void FetchMeSomeReader(Action<IDataReader> useReader)
{
    using(var reader = WhateverYouDoToMakeTheReader())
        useReader(reader);
}

Then in your calling class:

void Whatever()
{
   FetchMeSomeReader(SetFields);
}

void SetFields(IDataReader reader)
{
   MyListItem = new ListItem(datareader["dc"]);
}