Is it OK to pass DataReaders to constructors?

Question

I'm maintaining some C# 2.0 code and the programmer uses a pattern of reading a collection of business objects by opening a datareader and then passing it to the object's constructor. I can't see anything obviously wrong with this but it feels bad to me. Is this OK to do?

private static void GetObjects()
{
    List<MyObject> objects = new List<MyObject>();
    string sql = "Select ...";
    SqlConnection connection = GetConnection();
    SqlCommand command = new SqlCommand(sql, connection);
    connection.Open();
    SqlDataReader reader = command.ExecuteReader(CommandBehavior.CloseConnection);
    while (reader.Read())
        objects.Add(new MyObject(reader));
    reader.Close();
}

public MyObject(SqlDataReader reader)
{
    field0 = reader.GetString(0);
    field1 = reader.GetString(1);
    field2 = reader.GetString(2);
}

Answer 1

I would not do it this way, but I do not see anything majorly wrong.

Answer 2

Passing a reader around makes me cringe, unless it's a non-public helper method to deal with copying one row. Definitely not to a constructor though.

Passing a reader connects your constructor (you didn't put MyObject in a class but you call new MyObject()) to your data storage and I presume your object isn't written to be such?

If it were me:

private static void GetObjects()
{
    List<MyObject> objects = new List<MyObject>();
    string sql = "Select ...";
    using (SqlConnection connection = GetConnection())
    {
        SqlCommand command = new SqlCommand(sql, connection);
        connection.Open();
        using(SqlDataReader reader = command.ExecuteReader(CommandBehavior.CloseConnection);)
        {
            while (reader.Read())
                objects.Add(_ReadRow(reader));
        }
    }
}

private static MyObject _ReadRow(SqlDataReader reader)
{
    MyObject o = new MyObject();
    o.field0 = reader.GetString(0);
    o.field1 = reader.GetString(1);
    o.field2 = reader.GetString(2);

    // Do other manipulation to object before returning

    return o;
}

class MyObject{}

Answer 3

(EDIT: This answer focuses solely on the "what are the implications at a relatively low level" rather than the overall design implications. It looks like other answers have got those covered, so I won't comment :)

Well, there's definitely something wrong with the code you've given, as nothing closes the connection, command or reader. Specifically, your connection assignment line should usually look like this:

using (SqlConnection connection = GetConnection())
{
    ...
}

You may well think this is just nit-picking, and that it's just sample code and the clean-up is unimportant - but the "ownership" of resources which need cleaning up is precisely the issue with passing a DataReader to a constructor.

I think it's okay as long as you document who "owns" the reader afterwards. For instance, in Image.FromStream, the image owns the stream afterwards and may not take kindly to you closing it yourself (depending on the image format and a few other things). Other times it's still your responsibility to close. This must be very carefully documented, and if the type with the constructor takes ownership, it should implement IDisposable to make the clean-up easier and to make it more obvious that cleanup is required.

In your case, it looks like the constructor is not taking ownership of the reader, which is a perfectly fine (and simpler) alternative. Simply document that, and the caller will still have to close the reader appropriately.

Answer 4

I'd make entity passed in an IDataReader as this aids testing of the MyObject.

Answer 5

By passing the DataReader to the object's constructor, you establish a very tight coupling between the business object and your choice of persistence technology.

At the very least, this tight coupling will make reuse and testing difficult; at worst, it could result in the business objects knowing far too much about the database.

Resolving this isn't too difficult - you'd simply move object initialization out of the constructor and into a distinct factory class.

Answer 6

I'd call this a "leaky abstraction".

I like to use persistence objects in the narrowest scope possible: acquire them, use them, clean them up. If there was a well-defined persistence object, you could ask it to map the query result into an object or collection, close the reader within method scope, and return the object or collection to your client.

Answer 7

I totally agree with duffymo (+1) (and Bevan)

An idea I have been chewing over in my mind recently is, if I do have to have a dependency , and of course when I map a reader to an object I have to have a dependency, maybe I should make it completely explicit and in fact write an extension method to achieve it, something like...

//This Static extension class in the same namespace (but not necesarrily assembly) as my BO

public static class DataReaderExtensions
{
  public static List<MyObject> GetMyObjects(this DataReader reader)
  {

  }
}

this way, as long as I am in the reference scope of my business object, my datareader will now have a method tailored to my needs...the idea probably needs to be fleshed out more, but I think this would be elegant.