0

I have a function that returns an OracleDataReader object. Here it is :

 public OracleDataReader executeCommand(string query)
    {
        using (conn)
        {
            conn.ConnectionString = connectionString;
            conn.Open();
            OracleCommand cmd = conn.CreateCommand();
            cmd.CommandText = query;
            OracleDataReader reader = cmd.ExecuteReader();
            return reader;
        }
    }

When I tried to to get data with reader in another class i get an exception:

public Person GetPersonById(int id)
    {
        OracleDBOp db = new OracleDBOp();
        String query = "select * from test_person where id=" + id;
        OracleDataReader reader = db.executeCommand(query);
        Person person = null;
        person = new Person(Convert.ToInt32(reader["id"]),reader["first_name"].ToString(),reader["last_name"].ToString());
        return person;
    }

Exception
Invalid attempt to GetOrdinal when reader is closed.

What is the problem? I am not closing the reader?

StuartLC
  • 104,537
  • 17
  • 209
  • 285
mekafe
  • 596
  • 4
  • 13
  • 32

1 Answers1

4

Your code closes the connection as soon as the using scope is exited, so any attempts to read will fail:

using (conn)
{
   conn.ConnectionString = connectionString;
   conn.Open();
   ...
   return reader;
} --> Disposes of connection, which closes it, so reader can't read.

One option is to instead of placing the connection in the using block, if you intend passing the reader outside the control of the method which owns the connection, you can specify CommandBehavior.CloseConnection to close the connection when the reader is closed, but do NOT close or dispose of the connection i.e.

OracleDataReader reader = cmd.ExecuteReader(CommandBehavior.CloseConnection);
return reader;

IMO, a safer pattern (provided that you play nicely with the iterator, e.g. with foreach), without leaving the responsibility of the disposal of resources to the caller, and without passing the reader outside of a function, would be to use yield return, although this will change the way your code works:

  public IEnumerable<Person> RetrievePeople()
  {
      using (var conn = new OracleConnection(connString))
      {
        conn.Open();
        using (var cmd = conn.CreateCommand())
        {
          cmd.CommandText = query;
          using (var reader = cmd.ExecuteReader())
          {
            while (reader.Read())
            {
               yield return new new Person(
                 Convert.ToInt32(reader["id"]),
                 reader["first_name"].ToString(),
                 reader["last_name"].ToString());
            }
          }
        }
     }
  }

This way you don't need to pass the reader around, yet, the connection will be remain open until the iterator completes. This has the benefit of retaining the lazy evaluation approach of the reader, without actually passing the reader around, and without the issue of who is going to clean up the connections afterwards.

Community
  • 1
  • 1
StuartLC
  • 104,537
  • 17
  • 209
  • 285
  • 1
    Don't forget that `OracleDataReader` and OracleCommand` implement `IDisposable` – ta.speot.is Jan 31 '14 at 07:21
  • Thank you very much Stuart, but i have a question.Is this a good design that make the Person operations in a DB operations class? – mekafe Jan 31 '14 at 07:41
  • PS : Person operations will be bigger. – mekafe Jan 31 '14 at 07:41
  • The person can be regarded as a strongly typed DTO of sorts. Most ORM's encourage this kind of interaction. How far you pass the `Person` DTO up your architecture is up to you. – StuartLC Jan 31 '14 at 08:11